| Message ID | 1239895313-11841-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2009-04-16 at 11:21 -0400, Jeff Layton wrote: > The buffer for this was resized recently to fix a bug. It's still > possible however that a malicious server could overflow this field > by sending characters in it that are >2 bytes in the local charset. > Double the size of the buffer to account for this possibility. > > Also get rid of some really strange and seemingly pointless NULL > termination. It's NULL terminating the string in the source buffer, > but by the time that happens, we've already copied the string. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/connect.c | 7 ++----- > 1 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 01e280c..1a93604 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -3756,16 +3756,13 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, > BCC(smb_buffer_response)) { > kfree(tcon->nativeFileSystem); > tcon->nativeFileSystem = > - kzalloc(2*(length + 1), GFP_KERNEL); > + kzalloc((4 * length) + 2, GFP_KERNEL); > if (tcon->nativeFileSystem) > cifs_strfromUCS_le( > tcon->nativeFileSystem, > (__le16 *) bcc_ptr, > length, nls_codepage); > - bcc_ptr += 2 * length; > - bcc_ptr[0] = 0; /* null terminate the string */ > - bcc_ptr[1] = 0; > - bcc_ptr += 2; > + bcc_ptr += (2 * length) + 2; What's the point of updating bcc_ptr here? It's not accurate anyway. The correct thing would be: bcc_ptr += cifs_strfromUCS_le(... ); but bcc_ptr isn't used again, so there's no point. Shaggy
On Thu, 16 Apr 2009 15:41:33 +0000 Dave Kleikamp <shaggy@linux.vnet.ibm.com> wrote: > On Thu, 2009-04-16 at 11:21 -0400, Jeff Layton wrote: > > The buffer for this was resized recently to fix a bug. It's still > > possible however that a malicious server could overflow this field > > by sending characters in it that are >2 bytes in the local charset. > > Double the size of the buffer to account for this possibility. > > > > Also get rid of some really strange and seemingly pointless NULL > > termination. It's NULL terminating the string in the source buffer, > > but by the time that happens, we've already copied the string. > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/connect.c | 7 ++----- > > 1 files changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 01e280c..1a93604 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -3756,16 +3756,13 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, > > BCC(smb_buffer_response)) { > > kfree(tcon->nativeFileSystem); > > tcon->nativeFileSystem = > > - kzalloc(2*(length + 1), GFP_KERNEL); > > + kzalloc((4 * length) + 2, GFP_KERNEL); > > if (tcon->nativeFileSystem) > > cifs_strfromUCS_le( > > tcon->nativeFileSystem, > > (__le16 *) bcc_ptr, > > length, nls_codepage); > > - bcc_ptr += 2 * length; > > - bcc_ptr[0] = 0; /* null terminate the string */ > > - bcc_ptr[1] = 0; > > - bcc_ptr += 2; > > + bcc_ptr += (2 * length) + 2; > > What's the point of updating bcc_ptr here? It's not accurate anyway. > The correct thing would be: > > bcc_ptr += cifs_strfromUCS_le(... ); > > but bcc_ptr isn't used again, so there's no point. > You're right. There is no point. I just did it so that it would be updated in case someone did need to use it in the future, but it can safely be removed as long as that never happens.
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 01e280c..1a93604 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3756,16 +3756,13 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, BCC(smb_buffer_response)) { kfree(tcon->nativeFileSystem); tcon->nativeFileSystem = - kzalloc(2*(length + 1), GFP_KERNEL); + kzalloc((4 * length) + 2, GFP_KERNEL); if (tcon->nativeFileSystem) cifs_strfromUCS_le( tcon->nativeFileSystem, (__le16 *) bcc_ptr, length, nls_codepage); - bcc_ptr += 2 * length; - bcc_ptr[0] = 0; /* null terminate the string */ - bcc_ptr[1] = 0; - bcc_ptr += 2; + bcc_ptr += (2 * length) + 2; } /* else do not bother copying these information fields*/ } else {
The buffer for this was resized recently to fix a bug. It's still possible however that a malicious server could overflow this field by sending characters in it that are >2 bytes in the local charset. Double the size of the buffer to account for this possibility. Also get rid of some really strange and seemingly pointless NULL termination. It's NULL terminating the string in the source buffer, but by the time that happens, we've already copied the string. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/connect.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)