[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-***@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;

[Jeff Layton]

On Tue, 12 Aug 2014 09:19:42 -0500

Post by 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).
---
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...

Post by Steve French
+
+ /* 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.

Post by Steve French
return 1;
}
return 0;

[Jeremy Allison]

Post by Jeff Layton
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 :-).

Post by Jeff Layton
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 ?

[Steve French]

Post by Jeremy Allison

Post by Jeff Layton
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 :-).

Post by Jeff Layton
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).

[Jeff Layton]

On Thu, 14 Aug 2014 15:26:41 -0500

Post by Steve French

Post by Jeremy Allison

Post by Jeff Layton
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 :-).

Post by Jeff Layton
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?

[Jeremy Allison]

Post by Jeff Layton
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 !

Post by Jeff Layton
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Steve French]

Post by Jeremy Allison

Post by Jeff Layton
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 !

Post by Jeff Layton
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.

[Jeremy Allison]

Post by Steve French
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 :-).

Post by Steve French
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 :-).

[Steve French]

On Thu, Aug 14, 2014 at 2:30 PM, Jeff Layton

Post by Jeff Layton
On Tue, 12 Aug 2014 09:19:42 -0500

Post by 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).
---
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.

[Steve French]

Writes fail to Mac servers with SMB2.1 mounts (works with cifs though) due
to them sending an incorrect RFC1001 length for the SMB2.1 Write response.
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.

Incorporate feedback from Jeff and JRA to allow servers to send
a tcp frame that is even more than three bytes too long
(ie much longer than the SMB2/SMB3 request that it contains) but
we do log it once now. In the earlier version of the patch I had
limited how far off the length field could be before we fail the request.

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

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index f2e6ac2..4aa7a0f 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -178,9 +178,24 @@ smb2_check_message(char *buf, unsigned int length)
/* Windows 7 server returns 24 bytes more */
if (clc_len + 20 == len && command == SMB2_OPLOCK_BREAK_HE)
return 0;
- /* server can return one byte more */
+ /* server can return one byte more due to implied bcc[0] */
if (clc_len == 4 + len + 1)
return 0;
+
+ /*
+ * MacOS server pads after SMB2.1 write response with 3 bytes
+ * of junk. Other servers match RFC1001 len to actual
+ * SMB2/SMB3 frame length (header + smb2 response specific data)
+ * Log the server error (once), but allow it and continue
+ * since the frame is parseable.
+ */
+ if (clc_len < 4 /* RFC1001 header size */ + len) {
+ printk_once(KERN_WARNING
+ "SMB2 server sent bad RFC1001 len %d not %d\n",
+ len, clc_len - 4);
+ return 0;
+ }
+
return 1;
}
return 0;

[Shirish Pargaonkar]

Post by Steve French
Writes fail to Mac servers with SMB2.1 mounts (works with cifs though) due
to them sending an incorrect RFC1001 length for the SMB2.1 Write response.
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.
Incorporate feedback from Jeff and JRA to allow servers to send
a tcp frame that is even more than three bytes too long
(ie much longer than the SMB2/SMB3 request that it contains) but
we do log it once now. In the earlier version of the patch I had
limited how far off the length field could be before we fail the request.
---
fs/cifs/smb2misc.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index f2e6ac2..4aa7a0f 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -178,9 +178,24 @@ smb2_check_message(char *buf, unsigned int length)
/* Windows 7 server returns 24 bytes more */
if (clc_len + 20 == len && command == SMB2_OPLOCK_BREAK_HE)
return 0;
- /* server can return one byte more */
+ /* server can return one byte more due to implied bcc[0] */
if (clc_len == 4 + len + 1)
return 0;
+
+ /*
+ * MacOS server pads after SMB2.1 write response with 3 bytes
+ * of junk. Other servers match RFC1001 len to actual
+ * SMB2/SMB3 frame length (header + smb2 response specific data)
+ * Log the server error (once), but allow it and continue
+ * since the frame is parseable.
+ */
+ if (clc_len < 4 /* RFC1001 header size */ + len) {
+ printk_once(KERN_WARNING
+ "SMB2 server sent bad RFC1001 len %d not %d\n",
+ len, clc_len - 4);
+ return 0;
+ }
+
return 1;
}
return 0;
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html