Fixed. Included Pavel's comment as well.
On Wed, Dec 9, 2020 at 6:58 PM Tom Talpey <tom@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>
> >>> --
> >>> Best regards,
> >>> Pavel Shilovsky
> >>>
> >>> вт, 8 дек. 2020 г. в 23:23, Steve French <smfrench@xxxxxxxxx>:
> >>>>
> >>>> 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@xxxxxxxxxxxxxxx>
> >>>> Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
From b51801fb75f801e5436dc3f6889e1731ca4728e7 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@xxxxxxxxxxxxx>
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@xxxxxxxxxx>
Suggested-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
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
