[PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Steve French]

Writes fail to Mac servers with SMB2.1 mounts (works with cifs though) due
to them sending an incorrect RFC1001 length. Workaround this problem.
MacOS server sends a write response with 3 bytes of pad beyond the end
of the SMB itself.  The RFC1001 length is 3 bytes more than
the sum of the SMB2.1 header length + the write reponse.

Since we have seen a few other examples where servers have
padded responses strangely (oplock break and create),
allow servers to send a padded SMB2/SMB3 response to allow
for rounding (up to 15 bytes).

Signed-off-by: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/smb2misc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index f2e6ac2..da05beb 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -181,6 +181,12 @@ smb2_check_message(char *buf, unsigned int length)
         /* server can return one byte more */
         if (clc_len == 4 + len + 1)
             return 0;
+
+        /* MacOS server pads after SMB2.1 write response with 3 bytes */
+        /* of junk. Allow server to pad up to 15 bytes of junk at end */
+        if ((clc_len < 4 + len) && (clc_len > 4 + len - 16))
+            return 0;
+
         return 1;
     }
     return 0;

-- 
Thanks,

Steve

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Jeff Layton]

On Tue, 12 Aug 2014 09:19:42 -0500
Steve French <smfrench@gmail.com> wrote:

> Writes fail to Mac servers with SMB2.1 mounts (works with cifs though) due
> to them sending an incorrect RFC1001 length. Workaround this problem.
> MacOS server sends a write response with 3 bytes of pad beyond the end
> of the SMB itself.  The RFC1001 length is 3 bytes more than
> the sum of the SMB2.1 header length + the write reponse.
> 
> Since we have seen a few other examples where servers have
> padded responses strangely (oplock break and create),
> allow servers to send a padded SMB2/SMB3 response to allow
> for rounding (up to 15 bytes).
> 
> Signed-off-by: Steve French <smfrench@gmail.com>
> ---
>  fs/cifs/smb2misc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index f2e6ac2..da05beb 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -181,6 +181,12 @@ smb2_check_message(char *buf, unsigned int length)
>          /* server can return one byte more */
>          if (clc_len == 4 + len + 1)
>              return 0;

Not directly related to this patch, but...

What's the story behind the check above? Allowing the server to overrun
the rfc1001 length by one byte seems dangerous...

> +
> +        /* MacOS server pads after SMB2.1 write response with 3 bytes */
> +        /* of junk. Allow server to pad up to 15 bytes of junk at end */
> +        if ((clc_len < 4 + len) && (clc_len > 4 + len - 16))
> +            return 0;
> +

I don't understand the rationale for the arbitrary 15 byte limit. At
this point, you've already received the data. If there's extra junk at
the end, do you really care? I'd just ensure that clc_len fits within
the rfc1001 len and leave it at that.

>          return 1;
>      }
>      return 0;
> 


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Jeremy Allison]

On Thu, Aug 14, 2014 at 03:30:15PM -0400, Jeff Layton wrote:
> 
> Not directly related to this patch, but...
> 
> What's the story behind the check above? Allowing the server to overrun
> the rfc1001 length by one byte seems dangerous...

I vaguely remember a NetApp bug :-).

> I don't understand the rationale for the arbitrary 15 byte limit. At
> this point, you've already received the data. If there's extra junk at
> the end, do you really care? I'd just ensure that clc_len fits within
> the rfc1001 len and leave it at that.

+1 on this. No arbitrary limits please. If you're going
to ignore data after the valid packet, ignore everything
up to the rfc1001 length please. Only ignoring 15 bytes
doesn't make sense. Why 15 ? Why not 27 ?

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Steve French]

On Thu, Aug 14, 2014 at 2:35 PM, Jeremy Allison <jra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, Aug 14, 2014 at 03:30:15PM -0400, Jeff Layton wrote:
>>
>> Not directly related to this patch, but...
>>
>> What's the story behind the check above? Allowing the server to overrun
>> the rfc1001 length by one byte seems dangerous...
>
> I vaguely remember a NetApp bug :-).
>
>> I don't understand the rationale for the arbitrary 15 byte limit. At
>> this point, you've already received the data. If there's extra junk at
>> the end, do you really care? I'd just ensure that clc_len fits within
>> the rfc1001 len and leave it at that.
>
> +1 on this. No arbitrary limits please. If you're going
> to ignore data after the valid packet, ignore everything
> up to the rfc1001 length please. Only ignoring 15 bytes
> doesn't make sense. Why 15 ? Why not 27 ?

Mac is the only one with the length problem for SMB2 (actually SMB2.1), and
is 3 bytes over and only on one frame.  I am ok with doing the safest,
smallest fix (3 bytes) that gets it working.

But I don't like the idea of arbitrary screwed up RFC1001 length,
at worst I would prefer that we stay well within

MAX_SMB2_HDR_SIZE which is 0x78 (or about 0x30 bytes for a minimum
SMB2 response)

and certainly should never allow a RFC1001 length error to cause us to
alloc out of the
large buffer pool (although that would require a huge rounding error).
I also don't
like the idea of unverified data being sent to the client kernel (in
the space between
the end of the SMB3 response and the end of the TCP data).  Don't want
someone creative sticking code there.

I am fine with increasing it to only address the mac bug (ie 3 bytes)
or anything up to
0x30 bytes, but it complicates error checking if the RFC1001 length
can be off by
arbitrary amounts.  Realistically it is hard to imagine rounding
errors more than
sizeof(double) or 8 bytes.

The safest change is to only address the mac server bug (allow 3 bytes off).



-- 
Thanks,

Steve

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Jeff Layton]

On Thu, 14 Aug 2014 15:26:41 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Aug 14, 2014 at 2:35 PM, Jeremy Allison <jra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Thu, Aug 14, 2014 at 03:30:15PM -0400, Jeff Layton wrote:
> >>
> >> Not directly related to this patch, but...
> >>
> >> What's the story behind the check above? Allowing the server to overrun
> >> the rfc1001 length by one byte seems dangerous...
> >
> > I vaguely remember a NetApp bug :-).
> >
> >> I don't understand the rationale for the arbitrary 15 byte limit. At
> >> this point, you've already received the data. If there's extra junk at
> >> the end, do you really care? I'd just ensure that clc_len fits within
> >> the rfc1001 len and leave it at that.
> >
> > +1 on this. No arbitrary limits please. If you're going
> > to ignore data after the valid packet, ignore everything
> > up to the rfc1001 length please. Only ignoring 15 bytes
> > doesn't make sense. Why 15 ? Why not 27 ?
> 
> Mac is the only one with the length problem for SMB2 (actually SMB2.1), and
> is 3 bytes over and only on one frame.  I am ok with doing the safest,
> smallest fix (3 bytes) that gets it working.
> 
> But I don't like the idea of arbitrary screwed up RFC1001 length,
> at worst I would prefer that we stay well within
> 
> MAX_SMB2_HDR_SIZE which is 0x78 (or about 0x30 bytes for a minimum
> SMB2 response)
> 
> and certainly should never allow a RFC1001 length error to cause us to
> alloc out of the
> large buffer pool (although that would require a huge rounding error).
> I also don't
> like the idea of unverified data being sent to the client kernel (in
> the space between
> the end of the SMB3 response and the end of the TCP data).  Don't want
> someone creative sticking code there.
> 
> I am fine with increasing it to only address the mac bug (ie 3 bytes)
> or anything up to
> 0x30 bytes, but it complicates error checking if the RFC1001 length
> can be off by
> arbitrary amounts.  Realistically it is hard to imagine rounding
> errors more than
> sizeof(double) or 8 bytes.
> 
> The safest change is to only address the mac server bug (allow 3 bytes off).
> 
> 
> 

Failing here won't change the buffer allocation. That buffer has
already been allocated, and the receive is complete at this point. So
any "damage" has already been done.

So, I just don't get why you'd bother with an arbitrary limit at all.
The error checking is _simpler_ if you don't bother with this limit. Or
am I missing something here?

-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Jeremy Allison]

On Thu, Aug 14, 2014 at 04:40:14PM -0400, Jeff Layton wrote:
> 
> Failing here won't change the buffer allocation. That buffer has
> already been allocated, and the receive is complete at this point. So
> any "damage" has already been done.

Yep. You've got to have read the data to know it's too much !

> So, I just don't get why you'd bother with an arbitrary limit at all.
> The error checking is _simpler_ if you don't bother with this limit. Or
> am I missing something here?

Nope. The server has to deal with the same problem
as well. We just accept what the client sends inside
the NetBIOS length limit, and ignore anything after
the "useful" data within the packet.

Doesn't matter *what* is in the extra bits, code
data, whatever. We don't look at it.

Steve, what is the problem with just ignoring the
extra data ? If it offends you - log a warning
message is the rfc1001 length is too long and
ignore it.

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Steve French]

On Thu, Aug 14, 2014 at 3:44 PM, Jeremy Allison <jra-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, Aug 14, 2014 at 04:40:14PM -0400, Jeff Layton wrote:
>>
>> Failing here won't change the buffer allocation. That buffer has
>> already been allocated, and the receive is complete at this point. So
>> any "damage" has already been done.
>
> Yep. You've got to have read the data to know it's too much !
>
>> So, I just don't get why you'd bother with an arbitrary limit at all.
>> The error checking is _simpler_ if you don't bother with this limit. Or
>> am I missing something here?
>
> Nope. The server has to deal with the same problem
> as well. We just accept what the client sends inside
> the NetBIOS length limit, and ignore anything after
> the "useful" data within the packet.
>
> Doesn't matter *what* is in the extra bits, code
> data, whatever. We don't look at it.
>
> Steve, what is the problem with just ignoring the
> extra data ? If it offends you - log a warning
> message is the rfc1001 length is too long and
> ignore it.

It does bug me that the server would send
arbitrary amount of junk since it can slow
down the client.

I looked at the printk ratelimit stuff but
it looked like it was not worth the trouble
to avoid the log floods.

Perhaps just log if the length is off by
more than 3? So if some server is
off by more than the Mac write response
we can tell them.  Is it even worth the
trouble to ratelimit the log message if
we don't know of any server's that are off
by that much.


-- 
Thanks,

Steve

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Jeremy Allison]

On Thu, Aug 14, 2014 at 04:15:21PM -0500, Steve French wrote:
> 
> It does bug me that the server would send
> arbitrary amount of junk since it can slow
> down the client.

Well yes, it's definitely a server problem,
but there's nothing you can do about it unless
you decide you won't talk to servers that do
that :-).

> I looked at the printk ratelimit stuff but
> it looked like it was not worth the trouble
> to avoid the log floods.
> 
> Perhaps just log if the length is off by
> more than 3? So if some server is
> off by more than the Mac write response
> we can tell them.  Is it even worth the
> trouble to ratelimit the log message if
> we don't know of any server's that are off
> by that much.

More than 3 sounds good to me :-).

Re: [PATCH][CIFS] Workaround MacOS server problem with SMB2.1 write response

[Steve French]

On Thu, Aug 14, 2014 at 2:30 PM, Jeff Layton
<jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org> wrote:
> On Tue, 12 Aug 2014 09:19:42 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Writes fail to Mac servers with SMB2.1 mounts (works with cifs though) due
>> to them sending an incorrect RFC1001 length. Workaround this problem.
>> MacOS server sends a write response with 3 bytes of pad beyond the end
>> of the SMB itself.  The RFC1001 length is 3 bytes more than
>> the sum of the SMB2.1 header length + the write reponse.
>>
>> Since we have seen a few other examples where servers have
>> padded responses strangely (oplock break and create),
>> allow servers to send a padded SMB2/SMB3 response to allow
>> for rounding (up to 15 bytes).
>>
>> Signed-off-by: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/smb2misc.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
>> index f2e6ac2..da05beb 100644
>> --- a/fs/cifs/smb2misc.c
>> +++ b/fs/cifs/smb2misc.c
>> @@ -181,6 +181,12 @@ smb2_check_message(char *buf, unsigned int length)
>>          /* server can return one byte more */
>>          if (clc_len == 4 + len + 1)
>>              return 0;
>
> Not directly related to this patch, but...
>
> What's the story behind the check above? Allowing the server to overrun
> the rfc1001 length by one byte seems dangerous...

The problem IIRC was that at least one of the SMB2 requests/responses
was defined
with an implied data area which is at least 1 byte even if not present
(might be SMB2 read since struct size is odd, 17).  We had only run into
one other case (oplock break response) where length check was off,
so I prefer to stay stricter and safer if we don't know of any
interoperability issues.


-- 
Thanks,

Steve