| Message ID | CAH2r5msWAcy41S85b5eN5QBZSqwXDnQU07yHS0gnKE+zaaXw4A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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 ? -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 14, 2014 at 2:35 PM, Jeremy Allison <jra@samba.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).
On Thu, Aug 14, 2014 at 2:30 PM, Jeff Layton <jeff.layton@primarydata.com> wrote: > 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... 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.
On Thu, 14 Aug 2014 15:26:41 -0500 Steve French <smfrench@gmail.com> wrote: > On Thu, Aug 14, 2014 at 2:35 PM, Jeremy Allison <jra@samba.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?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 14, 2014 at 3:44 PM, Jeremy Allison <jra@samba.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.
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 :-). -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(+)