[PATCH] cifs: just ignore extra junk at the end of the SMB

[PATCH] cifs: just ignore extra junk at the end of the SMB

[Jeff Layton]

If the server sends us a RFC1001 length that's larger than the SMB,
then there's no reason to get our panties in a bunch and spew printk's,
and there's certainly no reason just ignore the response completely like
we do today. Just ignore the extra stuff on the end.

This fixes:

    https://bugzilla.samba.org/show_bug.cgi?id=7860

Reported-by: Marcus Schopen <marcus-dLPT46B32WUjnolme5KbmQ@public.gmane.org>
Tested-by: Burkhard Obergoeker <burkhard.obergoeker-VYBfkVvgaNUmRHQWxfeBDQ@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/misc.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 43f1028..b3df037 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -465,26 +465,13 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
 			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
 				return 0; /* bcc wrapped */
 		}
-		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
+
+		/*
+		 * We allow the server to send us an arbitrary amount of junk
+		 * at the end of the SMB. Just ignore it.
+		 */
+		cFYI(1, "Calculated size %u vs length %u mismatch for mid %u",
 				clc_len, 4 + len, smb->Mid);
-		/* Windows XP can return a few bytes too much, presumably
-		an illegal pad, at the end of byte range lock responses
-		so we allow for that three byte pad, as long as actual
-		received length is as long or longer than calculated length */
-		/* We have now had to extend this more, since there is a
-		case in which it needs to be bigger still to handle a
-		malformed response to transact2 findfirst from WinXP when
-		access denied is returned and thus bcc and wct are zero
-		but server says length is 0x21 bytes too long as if the server
-		forget to reset the smb rfc1001 length when it reset the
-		wct and bcc to minimum size and drop the t2 parms and data */
-		if ((4+len > clc_len) && (len <= clc_len + 512))
-			return 0;
-		else {
-			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
-					len, smb->Mid);
-			return 1;
-		}
 	}
 	return 0;
 }
-- 
1.7.3.3

Re: [PATCH] cifs: just ignore extra junk at the end of the SMB

[Suresh Jayaraman]

On 12/22/2010 07:09 PM, Jeff Layton wrote:
> If the server sends us a RFC1001 length that's larger than the SMB,
> then there's no reason to get our panties in a bunch and spew printk's,
> and there's certainly no reason just ignore the response completely like
> we do today. Just ignore the extra stuff on the end.
> 
> This fixes:
> 
>     https://bugzilla.samba.org/show_bug.cgi?id=7860
> 
> Reported-by: Marcus Schopen <marcus-dLPT46B32WUjnolme5KbmQ@public.gmane.org>
> Tested-by: Burkhard Obergoeker <burkhard.obergoeker-VYBfkVvgaNUmRHQWxfeBDQ@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/misc.c |   25 ++++++-------------------
>  1 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 43f1028..b3df037 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -465,26 +465,13 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
>  			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
>  				return 0; /* bcc wrapped */
>  		}
> -		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
> +
> +		/*
> +		 * We allow the server to send us an arbitrary amount of junk
> +		 * at the end of the SMB. Just ignore it.
> +		 */
> +		cFYI(1, "Calculated size %u vs length %u mismatch for mid %u",
>  				clc_len, 4 + len, smb->Mid);
> -		/* Windows XP can return a few bytes too much, presumably
> -		an illegal pad, at the end of byte range lock responses
> -		so we allow for that three byte pad, as long as actual
> -		received length is as long or longer than calculated length */
> -		/* We have now had to extend this more, since there is a
> -		case in which it needs to be bigger still to handle a
> -		malformed response to transact2 findfirst from WinXP when
> -		access denied is returned and thus bcc and wct are zero
> -		but server says length is 0x21 bytes too long as if the server
> -		forget to reset the smb rfc1001 length when it reset the
> -		wct and bcc to minimum size and drop the t2 parms and data */
> -		if ((4+len > clc_len) && (len <= clc_len + 512))
> -			return 0;
> -		else {
> -			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
> -					len, smb->Mid);
> -			return 1;
> -		}
>  	}
>  	return 0;
>  }

Where do we stand w.r.t this patch? Though it looks OK to me, IIRC,
Steve had some concerns in make the checks less stricter. Steve?


-- 
Suresh Jayaraman

Re: [PATCH] cifs: just ignore extra junk at the end of the SMB

[Jeff Layton]

On Fri, 07 Jan 2011 11:22:35 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 12/22/2010 07:09 PM, Jeff Layton wrote:
> > If the server sends us a RFC1001 length that's larger than the SMB,
> > then there's no reason to get our panties in a bunch and spew printk's,
> > and there's certainly no reason just ignore the response completely like
> > we do today. Just ignore the extra stuff on the end.
> > 
> > This fixes:
> > 
> >     https://bugzilla.samba.org/show_bug.cgi?id=7860
> > 
> > Reported-by: Marcus Schopen <marcus-dLPT46B32WUjnolme5KbmQ@public.gmane.org>
> > Tested-by: Burkhard Obergoeker <burkhard.obergoeker-VYBfkVvgaNUmRHQWxfeBDQ@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/misc.c |   25 ++++++-------------------
> >  1 files changed, 6 insertions(+), 19 deletions(-)
> > 
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index 43f1028..b3df037 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -465,26 +465,13 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
> >  			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
> >  				return 0; /* bcc wrapped */
> >  		}
> > -		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
> > +
> > +		/*
> > +		 * We allow the server to send us an arbitrary amount of junk
> > +		 * at the end of the SMB. Just ignore it.
> > +		 */
> > +		cFYI(1, "Calculated size %u vs length %u mismatch for mid %u",
> >  				clc_len, 4 + len, smb->Mid);
> > -		/* Windows XP can return a few bytes too much, presumably
> > -		an illegal pad, at the end of byte range lock responses
> > -		so we allow for that three byte pad, as long as actual
> > -		received length is as long or longer than calculated length */
> > -		/* We have now had to extend this more, since there is a
> > -		case in which it needs to be bigger still to handle a
> > -		malformed response to transact2 findfirst from WinXP when
> > -		access denied is returned and thus bcc and wct are zero
> > -		but server says length is 0x21 bytes too long as if the server
> > -		forget to reset the smb rfc1001 length when it reset the
> > -		wct and bcc to minimum size and drop the t2 parms and data */
> > -		if ((4+len > clc_len) && (len <= clc_len + 512))
> > -			return 0;
> > -		else {
> > -			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
> > -					len, smb->Mid);
> > -			return 1;
> > -		}
> >  	}
> >  	return 0;
> >  }
> 
> Where do we stand w.r.t this patch? Though it looks OK to me, IIRC,
> Steve had some concerns in make the checks less stricter. Steve?
> 
> 

Ahh, I haven't see where Steve commented on this patch...

I think this is safe though. The SMB packet sits inside of a RFC1001
"container". All this patch is saying is that if the RFC1001 container
is larger than the SMB then just ignore any remaining data after it.

Apparently, Netapps send some extra junk at the end of some SMBs and
that runs afoul of the above checks. The server is clearly broken in
this regard, but I see no reason for us to reject such packets outright
since we can easily deal with them.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: just ignore extra junk at the end of the SMB

[Suresh Jayaraman]

On 01/07/2011 07:04 PM, Jeff Layton wrote:
> On Fri, 07 Jan 2011 11:22:35 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 12/22/2010 07:09 PM, Jeff Layton wrote:
>>> If the server sends us a RFC1001 length that's larger than the SMB,
>>> then there's no reason to get our panties in a bunch and spew printk's,
>>> and there's certainly no reason just ignore the response completely like
>>> we do today. Just ignore the extra stuff on the end.
>>>
>>> This fixes:
>>>
>>>     https://bugzilla.samba.org/show_bug.cgi?id=7860
>>>
>>> Reported-by: Marcus Schopen <marcus-dLPT46B32WUjnolme5KbmQ@public.gmane.org>
>>> Tested-by: Burkhard Obergoeker <burkhard.obergoeker-VYBfkVvgaNUmRHQWxfeBDQ@public.gmane.org>
>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  fs/cifs/misc.c |   25 ++++++-------------------
>>>  1 files changed, 6 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
>>> index 43f1028..b3df037 100644
>>> --- a/fs/cifs/misc.c
>>> +++ b/fs/cifs/misc.c
>>> @@ -465,26 +465,13 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
>>>  			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
>>>  				return 0; /* bcc wrapped */
>>>  		}
>>> -		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
>>> +
>>> +		/*
>>> +		 * We allow the server to send us an arbitrary amount of junk
>>> +		 * at the end of the SMB. Just ignore it.
>>> +		 */
>>> +		cFYI(1, "Calculated size %u vs length %u mismatch for mid %u",
>>>  				clc_len, 4 + len, smb->Mid);
>>> -		/* Windows XP can return a few bytes too much, presumably
>>> -		an illegal pad, at the end of byte range lock responses
>>> -		so we allow for that three byte pad, as long as actual
>>> -		received length is as long or longer than calculated length */
>>> -		/* We have now had to extend this more, since there is a
>>> -		case in which it needs to be bigger still to handle a
>>> -		malformed response to transact2 findfirst from WinXP when
>>> -		access denied is returned and thus bcc and wct are zero
>>> -		but server says length is 0x21 bytes too long as if the server
>>> -		forget to reset the smb rfc1001 length when it reset the
>>> -		wct and bcc to minimum size and drop the t2 parms and data */
>>> -		if ((4+len > clc_len) && (len <= clc_len + 512))
>>> -			return 0;
>>> -		else {
>>> -			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
>>> -					len, smb->Mid);
>>> -			return 1;
>>> -		}
>>>  	}
>>>  	return 0;
>>>  }
>>
>> Where do we stand w.r.t this patch? Though it looks OK to me, IIRC,
>> Steve had some concerns in make the checks less stricter. Steve?
>>
>>
> 
> Ahh, I haven't see where Steve commented on this patch...

Not on this patch, but elsewhere while discussing an identical issue -
http://thread.gmane.org/gmane.network.samba.general/117198/focus=1986


-- 
Suresh Jayaraman

Re: [PATCH] cifs: just ignore extra junk at the end of the SMB

[Jeff Layton]

On Wed, 22 Dec 2010 08:39:07 -0500
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> If the server sends us a RFC1001 length that's larger than the SMB,
> then there's no reason to get our panties in a bunch and spew printk's,
> and there's certainly no reason just ignore the response completely like
> we do today. Just ignore the extra stuff on the end.
> 
> This fixes:
> 
>     https://bugzilla.samba.org/show_bug.cgi?id=7860
> 
> Reported-by: Marcus Schopen <marcus-dLPT46B32WUjnolme5KbmQ@public.gmane.org>
> Tested-by: Burkhard Obergoeker <burkhard.obergoeker-VYBfkVvgaNUmRHQWxfeBDQ@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/misc.c |   25 ++++++-------------------
>  1 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 43f1028..b3df037 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c

Self-NAK on this patch. It eliminates the check for a RFC header
that's too short to hold the calculated SMB lengths. After looking over
the captures, it appears that removing that check is what allowed this
to work against the server, which is sending SMB's with length fields
that go beyond the end of the response.

We'll need to rethink this approach. Steve suggested that we might be
able to "fix up" the BCC and trans2 data lengths, but we need to
consider whether that's reasonable to do in the general case.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>