[linux-cifs-client] [PATCH 4/4] cifs: verify lengths of QueryAllEAs reply

[Jeff Layton]

On Mon, 11 Jan 2010 15:57:44 -0600
Steve French <smfrench at gmail.com> wrote:

> The others look fine - has anyone proved that this fixes a problem in the
> field (if so, I would like to push it upstream).
> 
> On Mon, Jan 11, 2010 at 3:02 PM, Jeff Layton <jlayton at redhat.com> wrote:
> 
> > Make sure the lengths in a QUERY_ALL_EAS reply don't make the parser walk
> > off the end of the SMB.
> >
> > Signed-off-by: Jeff Layton <jlayton at redhat.com>
> > ---
> >  fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++---
> >  1 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index f5e1527..fb19f43 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -5285,6 +5285,7 @@ CIFSSMBQAllEAs(const int xid, struct cifsTconInfo
> > *tcon,
> >        struct fealist *ea_response_data;
> >        struct fea *temp_fea;
> >        char *temp_ptr;
> > +       char *end_of_smb;
> >        __u16 params, byte_count, data_offset;
> >
> >        cFYI(1, ("In Query All EAs path %s", searchName));
> > @@ -5360,11 +5361,19 @@ QAllEAsRetry:
> >        data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
> >        ea_response_data = (struct fealist *)
> >                                (((char *) &pSMBr->hdr.Protocol) +
> > data_offset);
> > -
> >        list_len = le32_to_cpu(ea_response_data->list_len);
> >        cFYI(1, ("ea length %d", list_len));
> >        if (list_len <= 8) {
> >                cFYI(1, ("empty EA list returned from server"));
> > +               rc = -EIO;
> > +               goto QAllEAsOut;
> > +       }
> > +
> > +       /* make sure list_len doesn't go past end of SMB */
> > +       end_of_smb = (char *)pByteArea(&pSMBr->hdr) + BCC(&pSMBr->hdr);
> > +       if ((char *)ea_response_data + list_len > end_of_smb) {
> > +               cFYI(1, ("list_len goes beyond SMB"));
> > +               rc = -EIO;
> >                goto QAllEAsOut;
> >        }
> >
> > @@ -5373,10 +5382,26 @@ QAllEAsRetry:
> >        temp_fea = ea_response_data->list;
> >        temp_ptr = (char *)temp_fea;
> >        while (list_len > 0) {
> > +               __u8 name_len;
> >                __u16 value_len;
> >                list_len -= 4;
> >                temp_ptr += 4;
> > -               rc += temp_fea->name_len;
> > +               /* make sure we can read name_len and value_len */
> > +               if (temp_ptr > end_of_smb) {
> > +                       cFYI(1, ("fealist goes beyond end of SMB"));
> > +                       rc = -EIO;
> > +                       goto QAllEAsOut;
> > +               }
> > +
> > +               name_len = temp_fea->name_len;
> > +               value_len = le16_to_cpu(temp_fea->value_len);
> > +               if (temp_ptr + name_len + value_len > end_of_smb) {
				   ^^^^^^^^
			erm...I think I forgot to figure in the null
			terminator here on the name. I'll respin and
			resend. In case it's not obvious, please be
			sure to sanity check my pointer math in this
			patch :)

> > +                       cFYI(1, ("fealist goes beyond end of SMB"));
> > +                       rc = -EIO;
> > +                       goto QAllEAsOut;
> > +               }
> > +
> > +               rc += name_len;
> >                /* account for prefix user. and trailing null */
> >                rc = rc + 5 + 1;
> >                if (rc < (int) buf_size) {
> > @@ -5399,7 +5424,6 @@ QAllEAsRetry:
> >                /* account for trailing null */
> >                list_len--;
> >                temp_ptr++;
> > -               value_len = le16_to_cpu(temp_fea->value_len);
> >                list_len -= value_len;
> >                temp_ptr += value_len;
> >                /* BB check that temp_ptr is still
> > --
> > 1.6.5.2
> >
> >
> 
> 


-- 
Jeff Layton <jlayton at redhat.com>