If a server has a bug and sends an SMB which is more than just "round up" off
in length - I can imagine a case for making exceptions depending on the
details of the type of malformed frame, but since we
already allow 512 bytes (for "incorrect server rounding of frame size" bugs)
we should be careful. If the length is more than 512 bytes off - this is
a server bug and the probability that the rest of the frame is incorrect
is very high. How off is the frame you want to allow.
No objection to fixing the error message - not clear on the other part.
Generally my intuition is that allowing a badly malformed response
without a clear server bug to workaround unnecessarily weakens
our validity checking. The better/stricter the validity checking the more
likely we are to throw away attack frames (intentionally illegal responses
designed to mess up the kernel).
On Thu, Jan 27, 2011 at 6:45 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> The cERROR message in checkSMB when the calculated length doesn't match
> the RFC1001 length is incorrect in many cases. It always says that the
> RFC1001 length is bigger than the SMB, even when it's actually the
> reverse.
>
> Fix the error message to say the reverse of what it does now and remove
> the arbitrary check when an RFC1001 length is larger than the SMB.
> There's no reason to reject those packets since we can just ignore the
> junk that's hanging off the end.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/misc.c | 19 +++----------------
> 1 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 72e99ec..959d629 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -467,23 +467,10 @@ 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",
> + 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",
> + if (4+len < clc_len) {
> + cERROR(1, "RFC1001 size %d smaller than SMB for mid=%u",
> len, smb->Mid);
> return 1;
> }
> --
> 1.7.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
