| Message ID | 1303819401-14789-4-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jeff Layton <jlayton@redhat.com> wrote: > - __u16 byte_count, total_data_size, total_in_buf, total_in_buf2; > + unsigned int byte_count, total_in_buf; > + __u16 total_data_size, total_in_buf2; There's no particular need for any of these to be __u16; I'd recommend making them all unsigned int or size_t. > + /* did this field "wrap" ? */ > + if (total_in_buf & ~((1<<16)-1)) > + return -EINVAL; I'd recommend something more like the following: /* check the server isn't offering too much data */ if (total_in_buf > USHRT_MAX) return -EINVAL; rather than calculating a mask. Also, would EPROTO be a better choice for packet parsing errors than EINVAL? > + /* did this field "wrap" ? */ > + if (byte_count & ~((1<<16)-1)) > + return -EINVAL; Ditto. David -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 26 Apr 2011 15:27:32 +0100 David Howells <dhowells@redhat.com> wrote: > Jeff Layton <jlayton@redhat.com> wrote: > > > - __u16 byte_count, total_data_size, total_in_buf, total_in_buf2; > > + unsigned int byte_count, total_in_buf; > > + __u16 total_data_size, total_in_buf2; > > There's no particular need for any of these to be __u16; I'd recommend making > them all unsigned int or size_t. > Fair enough. For those two, they can be __u16 with no problem, AFAICT. Is there some reason that an unsigned int would be better here? > > + /* did this field "wrap" ? */ > > + if (total_in_buf & ~((1<<16)-1)) > > + return -EINVAL; > > I'd recommend something more like the following: > > /* check the server isn't offering too much data */ > if (total_in_buf > USHRT_MAX) > return -EINVAL; > > rather than calculating a mask. > Ahh, didn't know about USHRT_MAX, I'll change it to use that instead. > Also, would EPROTO be a better choice for packet parsing errors than EINVAL? > > > + /* did this field "wrap" ? */ > > + if (byte_count & ~((1<<16)-1)) > > + return -EINVAL; > > Ditto. > > David Probably -- or EIO maybe. This error eventually gets discarded anyway once we mark the mid as malformed, so it really doesn't matter much for this. Still, more specific errors are better so maybe I'll change this when I respin it. Thanks for the review.
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index db9d55b..a038f29 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -274,7 +274,8 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) char *data_area_of_target; char *data_area_of_buf2; int remaining; - __u16 byte_count, total_data_size, total_in_buf, total_in_buf2; + unsigned int byte_count, total_in_buf; + __u16 total_data_size, total_in_buf2; total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); @@ -308,20 +309,28 @@ static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB) data_area_of_target += total_in_buf; /* copy second buffer into end of first buffer */ - memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2); total_in_buf += total_in_buf2; + /* did this field "wrap" ? */ + if (total_in_buf & ~((1<<16)-1)) + return -EINVAL; put_unaligned_le16(total_in_buf, &pSMBt->t2_rsp.DataCount); + byte_count = get_bcc_le(pTargetSMB); byte_count += total_in_buf2; + /* did this field "wrap" ? */ + if (byte_count & ~((1<<16)-1)) + return -EINVAL; put_bcc_le(byte_count, pTargetSMB); byte_count = pTargetSMB->smb_buf_length; byte_count += total_in_buf2; - - /* BB also add check that we are not beyond maximum buffer size */ - + /* don't allow buffer to overflow */ + if (byte_count > CIFSMaxBufSize) + return -EINVAL; pTargetSMB->smb_buf_length = byte_count; + memcpy(data_area_of_target, data_area_of_buf2, total_in_buf2); + if (remaining == total_in_buf2) { cFYI(1, "found the last secondary response"); return 0; /* we are done */
There are a couple of places in this code where these values can wrap or go negative, and that could potentially end up overflowing the buffer. Ensure that that doesn't happen. Do all of the length calculation and checks first, and only perform the memcpy after they pass. Also, increase some of the field sizes to 32 bits to ensure that they don't wrap without being detected. Cc: stable@kernel.org Reported-by: David Howells <dhowells@redhat.com> Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/connect.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-)