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: 3138 bytes --]

Fixed. Included Pavel's comment as well.


On Wed, Dec 9, 2020 at 6:58 PM Tom Talpey <tom@talpey.com> wrote:
>
> Except for the typo "wnich", looks fine.
>
> On 12/9/2020 5:49 PM, Steve French wrote:
> > Changed the comment in followon to:
> >
> > -       /* Make sure that negotiate contexts start after gss security blob */
> > +       /*
> > +        * if SPNEGO blob present (ie the RFC2478 GSS info which indicates
> > +        * wnich security mechanisms the server supports) make sure that
> > +        * the negotiate contexts start after it
> > +        */
> >
> > On Wed, Dec 9, 2020 at 3:26 PM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> The protocol allows omitting the SPNEGO blob altogether, btw. That
> >> leads to the client deciding how to authenticate, although the Windows
> >> server doesn't offer that.
> >>
> >> So I'd suggest removing the comment, too:
> >>
> >>   >> /* Make sure that negotiate contexts start after gss security blob */
> >>
> >>
> >> On 12/9/2020 12:39 PM, Pavel Shilovsky wrote:
> >>> Looks good.
> >>>
> >>> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >>>
> >>> --
> >>> Best regards,
> >>> Pavel Shilovsky
> >>>
> >>> вт, 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;
> >>>>
> >>>>     /* Verify that at least minimal negotiate contexts fit within frame */
> >>>>     if (len < nc_offset + (neg_count * sizeof(struct smb2_neg_context))) {
> >>>>
> >>>> --
> >>>> Thanks,
> >>>>
> >>>> Steve
> >>>
> >
> >
> >



-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-update-comments-clarifying-SPNEGO-info-in-n.patch --]
[-- Type: text/x-patch, Size: 1514 bytes --]

From b51801fb75f801e5436dc3f6889e1731ca4728e7 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 9 Dec 2020 21:25:13 -0600
Subject: [PATCH] SMB3.1.1: update comments clarifying SPNEGO info in negprot
 response

Trivial changes to clarify confusing comment (and also length comparisons
in negotiate context parsing.

Suggested-by: Tom Talpey <tom@talpey.com>
Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2misc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 2f86c1207a1f..8bd3b33cc0ad 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -107,8 +107,17 @@ static __u32 get_neg_ctxt_len(struct smb2_sync_hdr *hdr, __u32 len,
 	   (pneg_rsp->DialectRevision != cpu_to_le16(SMB311_PROT_ID)))
 		return 0;
 
-	/* Make sure that negotiate contexts start after gss security blob */
+	/*
+	 * if SPNEGO blob present (ie the RFC2478 GSS info which indicates
+	 * which security mechanisms the server supports) make sure that
+	 * the negotiate contexts start after it
+	 */
 	nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
+	/*
+	 * non_ctxlen is shdr->StructureSize + pdu->StructureSize2 and
+	 * the latter is 1 byte bigger than the fix-sized area of the
+	 * NEGOTIATE response
+	 */
 	if (nc_offset + 1 < non_ctxlen) {
 		pr_warn_once("Invalid negotiate context offset %d\n", nc_offset);
 		return 0;
-- 
2.27.0