| Message ID | 1314098488-1547-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
merged On Tue, Aug 23, 2011 at 6:21 AM, Jeff Layton <jlayton@redhat.com> wrote: > The name_len variable in CIFSFindNext is a signed int that gets set to > the resume_name_len in the cifs_search_info. The resume_name_len however > is unsigned and for some infolevels is populated directly from a 32 bit > value sent by the server. > > If the server sends a very large value for this, then that value could > look negative when converted to a signed int. That would make that > value pass the PATH_MAX check later in CIFSFindNext. The name_len would > then be used as a length value for a memcpy. It would then be treated > as unsigned again, and the memcpy scribbles over a ton of memory. > > Fix this by making the name_len an unsigned value in CIFSFindNext. > > Cc: <stable@kernel.org> > Reported-by: Darren Lavender <dcl@hppine99.gbr.hp.com> > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/cifssmb.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index f4d0988..950464d 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -4089,7 +4089,8 @@ int CIFSFindNext(const int xid, struct cifs_tcon *tcon, > T2_FNEXT_RSP_PARMS *parms; > char *response_data; > int rc = 0; > - int bytes_returned, name_len; > + int bytes_returned; > + unsigned int name_len; > __u16 params, byte_count; > > cFYI(1, "In FindNext"); > -- > 1.7.6 > >
On 08/23/2011 04:51 PM, Jeff Layton wrote: > The name_len variable in CIFSFindNext is a signed int that gets set to > the resume_name_len in the cifs_search_info. The resume_name_len however > is unsigned and for some infolevels is populated directly from a 32 bit > value sent by the server. > > If the server sends a very large value for this, then that value could > look negative when converted to a signed int. That would make that > value pass the PATH_MAX check later in CIFSFindNext. The name_len would > then be used as a length value for a memcpy. It would then be treated > as unsigned again, and the memcpy scribbles over a ton of memory. > > Fix this by making the name_len an unsigned value in CIFSFindNext. > > Cc: <stable@kernel.org> > Reported-by: Darren Lavender <dcl@hppine99.gbr.hp.com> > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/cifssmb.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index f4d0988..950464d 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -4089,7 +4089,8 @@ int CIFSFindNext(const int xid, struct cifs_tcon *tcon, > T2_FNEXT_RSP_PARMS *parms; > char *response_data; > int rc = 0; > - int bytes_returned, name_len; > + int bytes_returned; > + unsigned int name_len; Looks obviously correct. Just curious when does the server sends a very large resume_name_len? Does it try to overload with some other info in such circumstances?
On Tue, 23 Aug 2011 17:55:25 +0530 Suresh Jayaraman <sjayaraman@suse.de> wrote: > On 08/23/2011 04:51 PM, Jeff Layton wrote: > > The name_len variable in CIFSFindNext is a signed int that gets set to > > the resume_name_len in the cifs_search_info. The resume_name_len however > > is unsigned and for some infolevels is populated directly from a 32 bit > > value sent by the server. > > > > If the server sends a very large value for this, then that value could > > look negative when converted to a signed int. That would make that > > value pass the PATH_MAX check later in CIFSFindNext. The name_len would > > then be used as a length value for a memcpy. It would then be treated > > as unsigned again, and the memcpy scribbles over a ton of memory. > > > > Fix this by making the name_len an unsigned value in CIFSFindNext. > > > > Cc: <stable@kernel.org> > > Reported-by: Darren Lavender <dcl@hppine99.gbr.hp.com> > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/cifssmb.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > > index f4d0988..950464d 100644 > > --- a/fs/cifs/cifssmb.c > > +++ b/fs/cifs/cifssmb.c > > @@ -4089,7 +4089,8 @@ int CIFSFindNext(const int xid, struct cifs_tcon *tcon, > > T2_FNEXT_RSP_PARMS *parms; > > char *response_data; > > int rc = 0; > > - int bytes_returned, name_len; > > + int bytes_returned; > > + unsigned int name_len; > > Looks obviously correct. Just curious when does the server sends a very > large resume_name_len? Does it try to overload with some other info in > such circumstances? > It's hard to imagine that a properly functioning server ever would. More likely this would be the result of a malicious or broken server. We have a case where this occurred but it's unfortunately marked private so I can't share the details. In that situation though I believe the environment was using an asian language codepage on the server. My suspicions is that this may have happened as the result of another client or server bug that threw the alignment of the response off.
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index f4d0988..950464d 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -4089,7 +4089,8 @@ int CIFSFindNext(const int xid, struct cifs_tcon *tcon, T2_FNEXT_RSP_PARMS *parms; char *response_data; int rc = 0; - int bytes_returned, name_len; + int bytes_returned; + unsigned int name_len; __u16 params, byte_count; cFYI(1, "In FindNext");
The name_len variable in CIFSFindNext is a signed int that gets set to the resume_name_len in the cifs_search_info. The resume_name_len however is unsigned and for some infolevels is populated directly from a 32 bit value sent by the server. If the server sends a very large value for this, then that value could look negative when converted to a signed int. That would make that value pass the PATH_MAX check later in CIFSFindNext. The name_len would then be used as a length value for a memcpy. It would then be treated as unsigned again, and the memcpy scribbles over a ton of memory. Fix this by making the name_len an unsigned value in CIFSFindNext. Cc: <stable@kernel.org> Reported-by: Darren Lavender <dcl@hppine99.gbr.hp.com> Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/cifssmb.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)