Re: [PATCH][SMB3.1.1] remove confusing mount warning when no SPNEGO info on negprot rsp

[Steve French <smfrench@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

updated patch with Pavel's suggestion


On Fri, Dec 11, 2020 at 12:37 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 10 дек. 2020 г. в 09:45, Pavel Shilovsky <piastryyy@gmail.com>:
> >
> > вт, 8 дек. 2020 г. в 23:23, Steve French <smfrench@gmail.com>:
> > >
> > > Azure does not send an SPNEGO blob in the negotiate protocol response,
> > > so we shouldn't assume that it is there when validating the location
> > > of the first negotiate context.  This avoids the potential confusing
> > > mount warning:
> > >
> > >    CIFS: Invalid negotiate context offset
> > >
> > > CC: Stable <stable@vger.kernel.org>
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/smb2misc.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> > > index d88e2683626e..513507e4c4ad 100644
> > > --- a/fs/cifs/smb2misc.c
> > > +++ b/fs/cifs/smb2misc.c
> > > @@ -109,11 +109,14 @@ static __u32 get_neg_ctxt_len(struct
> > > smb2_sync_hdr *hdr, __u32 len,
> > >
> > >   /* Make sure that negotiate contexts start after gss security blob */
> > >   nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
> > > - if (nc_offset < non_ctxlen) {
> > > - pr_warn_once("Invalid negotiate context offset\n");
> > > + if (nc_offset + 1 < non_ctxlen) {
> > > + pr_warn_once("Invalid negotiate context offset %d\n", nc_offset);
> > >   return 0;
> > > - }
> > > - size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen;
> > > + } else if (nc_offset + 1 == non_ctxlen) {
> > > + cifs_dbg(FYI, "no SPNEGO security blob in negprot rsp\n");
> > > + size_of_pad_before_neg_ctxts = 0;
> > > + } else
> > > + size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen;
> > >
> >
> > This seems missing "+1" in the line above (non_ctxlen is 1 byte bigger
> > than the fix-sized area of the packet):
> > size_of_pad_before_neg_ctxts = nc_offset + 1 - non_ctxlen;
> >
>
> It seems that +1 would be needed if there is no SPNEGO security blob
> but negotiate context offset is padded for other reasons. In this case
> non_ctxlen will account for 1 byte from the padding. The only way here
> would be to do something like:
>
>  size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen + (non_ctxlen
> == 65 ? 1 : 0);
>
> --
> Best regards,
> Pavel Shilovsky



-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-remove-confusing-mount-warning-when-no-SPNE.patch --]
[-- Type: text/x-patch, Size: 2252 bytes --]

From b24a744b98cec59db80018d1efe937306a2c81d1 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 9 Dec 2020 01:12:35 -0600
Subject: [PATCH] SMB3.1.1: remove confusing mount warning when no SPNEGO info
 on negprot rsp

Azure does not send an SPNEGO blob in the negotiate protocol response,
so we shouldn't assume that it is there when validating the location
of the first negotiate context.  This avoids the potential confusing
mount warning:

   CIFS: Invalid negotiate context offset

CC: Stable <stable@vger.kernel.org>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2misc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index d88e2683626e..2da6b41cb552 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -94,6 +94,8 @@ static const __le16 smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
 	/* SMB2_OPLOCK_BREAK */ cpu_to_le16(24)
 };
 
+#define SMB311_NEGPROT_BASE_SIZE (sizeof(struct smb2_sync_hdr) + sizeof(struct smb2_negotiate_rsp))
+
 static __u32 get_neg_ctxt_len(struct smb2_sync_hdr *hdr, __u32 len,
 			      __u32 non_ctxlen)
 {
@@ -109,11 +111,17 @@ static __u32 get_neg_ctxt_len(struct smb2_sync_hdr *hdr, __u32 len,
 
 	/* Make sure that negotiate contexts start after gss security blob */
 	nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
-	if (nc_offset < non_ctxlen) {
-		pr_warn_once("Invalid negotiate context offset\n");
+	if (nc_offset + 1 < non_ctxlen) {
+		pr_warn_once("Invalid negotiate context offset %d\n", nc_offset);
 		return 0;
-	}
-	size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen;
+	} else if (nc_offset + 1 == non_ctxlen) {
+		cifs_dbg(FYI, "no SPNEGO security blob in negprot rsp\n");
+		size_of_pad_before_neg_ctxts = 0;
+	} else if (non_ctxlen == SMB311_NEGPROT_BASE_SIZE)
+		/* has padding, but no SPNEGO blob */
+		size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen + 1;
+	else
+		size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen;
 
 	/* Verify that at least minimal negotiate contexts fit within frame */
 	if (len < nc_offset + (neg_count * sizeof(struct smb2_neg_context))) {
-- 
2.27.0