| Message ID | 1263243721-3782-5-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
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@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@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) { > + 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 > >
On Mon, 11 Jan 2010 15:57:44 -0600 Steve French <smfrench@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). > Unfortunately, no. I held out hope for a while that I'd get a capture that showed one of these malformed responses, but was never able to get one. The reporter also wasn't keen on testing patches either, so I have no confirmation that this fixes anything. > On Mon, Jan 11, 2010 at 3:02 PM, Jeff Layton <jlayton@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@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) { > > + 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 > > > > > >
On Mon, 11 Jan 2010 15:57:44 -0600 Steve French <smfrench@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@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@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 > > > > > >
On Mon, Jan 11, 2010 at 7:00 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 11 Jan 2010 15:57:44 -0600 > Steve French <smfrench@gmail.com> wrote: > > > > + } > > > + > > > + 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 :) > > ok ... :)
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) { + 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
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@redhat.com> --- fs/cifs/cifssmb.c | 30 +++++++++++++++++++++++++++--- 1 files changed, 27 insertions(+), 3 deletions(-)