| Message ID | CAH2r5mvr6JebH9cr9dO-XbiXdsfBjs=C4WhqkqXUwDCmOY20zA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [SMB3.1.1] remove confusing mount warning when no SPNEGO info on negprot rsp | expand |
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
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 >
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 > >
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 >>> > > >
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 > >>> > > > > > >
вт, 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
чт, 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
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
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
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(-) if (len < nc_offset + (neg_count * sizeof(struct smb2_neg_context))) {