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

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

[Steve French]

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

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

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

From a26c67744b1ad06209dbf0b37aac306c1f3c7a8d 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>
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))) {
-- 
2.27.0

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

[Pavel Shilovsky]

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

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

[Tom Talpey]

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
> 

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

[Steve French]

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

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

[Tom Talpey]

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
>>>
> 
> 
> 

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

[Steve French]

[-- 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

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

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

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;

--
Best regards,
Pavel Shilovsky

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

[Pavel Shilovsky]

чт, 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

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

[Steve French]

[-- 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