[PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Pratyush Yadav]

smb2_queryfs() calls smb2_query_info_compound() with cifs_sb set to
NULL. It is then dereferenced by cifs_create_options(). Commit
a6e44cb21534d ("SMB3: Backup intent flag missing from some more ops")
removed the NULL check before dereferencing cifs_sb. Add it back.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: a6e44cb21534d ("SMB3: Backup intent flag missing from some more ops")
Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
---

Only compile-tested. I do not know this code very well. This was pointed
out by our static code analysis tool.

 fs/cifs/smb2ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 4cb0ebe7330eb..04256edaa4f73 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -2272,7 +2272,10 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
 	oparms.tcon = tcon;
 	oparms.desired_access = desired_access;
 	oparms.disposition = FILE_OPEN;
-	oparms.create_options = cifs_create_options(cifs_sb, 0);
+	if (cifs_sb)
+		oparms.create_options = cifs_create_options(cifs_sb, 0);
+	else
+		oparms.create_options = 0;
 	oparms.fid = &fid;
 	oparms.reconnect = false;

--
2.39.2

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
Subject: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()
Link: https://lore.kernel.org/stable/20230405114220.108739-1-ptyadav%40amazon.de

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Pratyush Yadav]

On Wed, Apr 05 2023, kernel test robot wrote:

> Hi,
>
> Thanks for your patch.
>
> FYI: kernel test robot notices the stable kernel rule is not satisfied.
>
> Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'

I think the robot should also learn to look at the 'To:' header :-)

> Subject: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()
> Link: https://lore.kernel.org/stable/20230405114220.108739-1-ptyadav%40amazon.de
>
> The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Greg KH]

On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
> On Wed, Apr 05 2023, kernel test robot wrote:
> 
> > Hi,
> >
> > Thanks for your patch.
> >
> > FYI: kernel test robot notices the stable kernel rule is not satisfied.
> >
> > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
> 
> I think the robot should also learn to look at the 'To:' header :-)

Nope, the robot is correct, you submitted this incorrectly.

thanks,

greg k-h

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Greg KH]

On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
> > On Wed, Apr 05 2023, kernel test robot wrote:
> > 
> > > Hi,
> > >
> > > Thanks for your patch.
> > >
> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
> > >
> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
> > 
> > I think the robot should also learn to look at the 'To:' header :-)
> 
> Nope, the robot is correct, you submitted this incorrectly.

Wait, maybe, I can't tell.  Please send this again and provide a whole
lot more detail as to why this is not relevant for upstream.

thanks,

greg k-h

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Pratyush Yadav]

On Wed, Apr 05 2023, Greg KH wrote:

> On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
>> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
>> > On Wed, Apr 05 2023, kernel test robot wrote:
>> >
>> > > Hi,
>> > >
>> > > Thanks for your patch.
>> > >
>> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
>> > >
>> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
>> >
>> > I think the robot should also learn to look at the 'To:' header :-)
>>
>> Nope, the robot is correct, you submitted this incorrectly.
>
> Wait, maybe, I can't tell.

My point is that it does not matter much if stable@vger.kernel.org is in
Cc or To. It gets the email regardless. In fact, that seems quite a
common practice to me [0][1]. So I'd say it would be nice if the robot
did not needlessly complain about this.

> Please send this again and provide a whole lot more detail as to why
> this is not relevant for upstream.

I went and took another look. It seems that this was also fixed in
upstream but in a slightly different way [2]. I will backport that patch
instead of this one.

[0] https://lore.kernel.org/stable/20230403140414.236685532@linuxfoundation.org/
[1] https://lore.kernel.org/stable/20230403140415.140110769@linuxfoundation.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87f93d82e0952da18af4d978e7d887b4c5326c0b

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Greg KH]

On Wed, Apr 05, 2023 at 03:33:20PM +0200, Pratyush Yadav wrote:
> On Wed, Apr 05 2023, Greg KH wrote:
> 
> > On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
> >> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
> >> > On Wed, Apr 05 2023, kernel test robot wrote:
> >> >
> >> > > Hi,
> >> > >
> >> > > Thanks for your patch.
> >> > >
> >> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
> >> > >
> >> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
> >> >
> >> > I think the robot should also learn to look at the 'To:' header :-)
> >>
> >> Nope, the robot is correct, you submitted this incorrectly.
> >
> > Wait, maybe, I can't tell.
> 
> My point is that it does not matter much if stable@vger.kernel.org is in
> Cc or To. It gets the email regardless. In fact, that seems quite a
> common practice to me [0][1]. So I'd say it would be nice if the robot
> did not needlessly complain about this.

The robot replaces my bot (well, aguments this), and it rightfully flags
many patches that are sent to stable that are not done so correctly, so
that the submitter can then fix them up.  The number of "false
positives" like this is pretty low, as hey, even I got it wrong when
reading this "by hand".

thanks,

greg k-h

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Yujie Liu]

Hi Greg, Hi Pratyush,

On Wed, Apr 05, 2023 at 04:22:58PM +0200, Greg KH wrote:
> On Wed, Apr 05, 2023 at 03:33:20PM +0200, Pratyush Yadav wrote:
> > On Wed, Apr 05 2023, Greg KH wrote:
> > 
> > > On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
> > >> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
> > >> > On Wed, Apr 05 2023, kernel test robot wrote:
> > >> >
> > >> > > Hi,
> > >> > >
> > >> > > Thanks for your patch.
> > >> > >
> > >> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
> > >> > >
> > >> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'

Sorry the info at here is not accurate enough. We will improve the
wording.

> > >> >
> > >> > I think the robot should also learn to look at the 'To:' header :-)
> > >>
> > >> Nope, the robot is correct, you submitted this incorrectly.
> > >
> > > Wait, maybe, I can't tell.
> > 
> > My point is that it does not matter much if stable@vger.kernel.org is in
> > Cc or To. It gets the email regardless. In fact, that seems quite a
> > common practice to me [0][1]. So I'd say it would be nice if the robot
> > did not needlessly complain about this.
> >
> > [0] https://lore.kernel.org/stable/20230403140414.236685532@linuxfoundation.org/
> > [1] https://lore.kernel.org/stable/20230403140415.140110769@linuxfoundation.org/
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87f93d82e0952da18af4d978e7d887b4c5326c0b

This warning is not caused by "stable@vger.kernel.org is in To or Cc".

The document at [3] gives three options for sending patches to stable,
and seems option 3 should apply on this patch:

[3] https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Option 3

Send the patch, after verifying that it follows the above rules, to stable@vger.kernel.org. You must note the upstream commit ID in the changelog of your submission, as well as the kernel version you wish it to be applied to.

The examples in link [0][1] have "upstream commit" in the changelog, but
this patch doesn't, so the robot flags a warning.

> The robot replaces my bot (well, aguments this), and it rightfully flags
> many patches that are sent to stable that are not done so correctly, so
> that the submitter can then fix them up.  The number of "false
> positives" like this is pretty low, as hey, even I got it wrong when
> reading this "by hand".

Thanks for the affirmation of our robot. Could you help give some
suggestions so we can further improve the robot to reduce "false
positives"? Do we still need to check "upstream commit" in changelog for
similar cases?

--
Best Regards,
Yujie

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Pratyush Yadav]

On Wed, Apr 12 2023, Yujie Liu wrote:

> Hi Greg, Hi Pratyush,
>
> On Wed, Apr 05, 2023 at 04:22:58PM +0200, Greg KH wrote:
>> On Wed, Apr 05, 2023 at 03:33:20PM +0200, Pratyush Yadav wrote:
>> > On Wed, Apr 05 2023, Greg KH wrote:
>> >
>> > > On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
>> > >> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
>> > >> > On Wed, Apr 05 2023, kernel test robot wrote:
>> > >> >
>> > >> > > Hi,
>> > >> > >
>> > >> > > Thanks for your patch.
>> > >> > >
>> > >> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
>> > >> > >
>> > >> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
>
> Sorry the info at here is not accurate enough. We will improve the
> wording.
>
>> > >> >
>> > >> > I think the robot should also learn to look at the 'To:' header :-)
>> > >>
>> > >> Nope, the robot is correct, you submitted this incorrectly.
>> > >
>> > > Wait, maybe, I can't tell.
>> >
>> > My point is that it does not matter much if stable@vger.kernel.org is in
>> > Cc or To. It gets the email regardless. In fact, that seems quite a
>> > common practice to me [0][1]. So I'd say it would be nice if the robot
>> > did not needlessly complain about this.
>> >
>> > [0] https://lore.kernel.org/stable/20230403140414.236685532@linuxfoundation.org/
>> > [1] https://lore.kernel.org/stable/20230403140415.140110769@linuxfoundation.org/
>> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87f93d82e0952da18af4d978e7d887b4c5326c0b
>
> This warning is not caused by "stable@vger.kernel.org is in To or Cc".
>
> The document at [3] gives three options for sending patches to stable,
> and seems option 3 should apply on this patch:
>
> [3] https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> Option 3
>
> Send the patch, after verifying that it follows the above rules, to stable@vger.kernel.org. You must note the upstream commit ID in the changelog of your submission, as well as the kernel version you wish it to be applied to.
>
> The examples in link [0][1] have "upstream commit" in the changelog, but
> this patch doesn't, so the robot flags a warning.

It is entirely possible for a patch for a stable tree to not have an
upstream commit. For example, I sent a patch recently [0] that was
caused by a buggy backport. The patch to fix it of course would not have
an upstream commit since upstream was correct from the get-go. The bot
should not complain about such patches.

Funnily enough the bot did not complain there even though that patch
also does not have an upstream commit hash. But it puts
stable@vger.kernel.org in Cc instead of To.

[0] https://lore.kernel.org/all/20230411130210.113555-1-ptyadav@amazon.de/

>
>> The robot replaces my bot (well, aguments this), and it rightfully flags
>> many patches that are sent to stable that are not done so correctly, so
>> that the submitter can then fix them up.  The number of "false
>> positives" like this is pretty low, as hey, even I got it wrong when
>> reading this "by hand".
>
> Thanks for the affirmation of our robot. Could you help give some
> suggestions so we can further improve the robot to reduce "false
> positives"? Do we still need to check "upstream commit" in changelog for
> similar cases?
>
> --
> Best Regards,
> Yujie

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 5.4] cifs/smb3: Fix NULL pointer dereference in smb2_query_info_compound()

[Yujie Liu]

On Wed, Apr 12, 2023 at 03:21:34PM +0200, Pratyush Yadav wrote:
> On Wed, Apr 12 2023, Yujie Liu wrote:
> 
> > Hi Greg, Hi Pratyush,
> >
> > On Wed, Apr 05, 2023 at 04:22:58PM +0200, Greg KH wrote:
> >> On Wed, Apr 05, 2023 at 03:33:20PM +0200, Pratyush Yadav wrote:
> >> > On Wed, Apr 05 2023, Greg KH wrote:
> >> >
> >> > > On Wed, Apr 05, 2023 at 02:26:04PM +0200, Greg KH wrote:
> >> > >> On Wed, Apr 05, 2023 at 01:47:52PM +0200, Pratyush Yadav wrote:
> >> > >> > On Wed, Apr 05 2023, kernel test robot wrote:
> >> > >> >
> >> > >> > > Hi,
> >> > >> > >
> >> > >> > > Thanks for your patch.
> >> > >> > >
> >> > >> > > FYI: kernel test robot notices the stable kernel rule is not satisfied.
> >> > >> > >
> >> > >> > > Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
> >
> > Sorry the info at here is not accurate enough. We will improve the
> > wording.
> >
> >> > >> >
> >> > >> > I think the robot should also learn to look at the 'To:' header :-)
> >> > >>
> >> > >> Nope, the robot is correct, you submitted this incorrectly.
> >> > >
> >> > > Wait, maybe, I can't tell.
> >> >
> >> > My point is that it does not matter much if stable@vger.kernel.org is in
> >> > Cc or To. It gets the email regardless. In fact, that seems quite a
> >> > common practice to me [0][1]. So I'd say it would be nice if the robot
> >> > did not needlessly complain about this.
> >> >
> >> > [0] https://lore.kernel.org/stable/20230403140414.236685532@linuxfoundation.org/
> >> > [1] https://lore.kernel.org/stable/20230403140415.140110769@linuxfoundation.org/
> >> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87f93d82e0952da18af4d978e7d887b4c5326c0b
> >
> > This warning is not caused by "stable@vger.kernel.org is in To or Cc".
> >
> > The document at [3] gives three options for sending patches to stable,
> > and seems option 3 should apply on this patch:
> >
> > [3] https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> >
> > Option 3
> >
> > Send the patch, after verifying that it follows the above rules, to stable@vger.kernel.org. You must note the upstream commit ID in the changelog of your submission, as well as the kernel version you wish it to be applied to.
> >
> > The examples in link [0][1] have "upstream commit" in the changelog, but
> > this patch doesn't, so the robot flags a warning.
> 
> It is entirely possible for a patch for a stable tree to not have an
> upstream commit. For example, I sent a patch recently [0] that was
> caused by a buggy backport. The patch to fix it of course would not have
> an upstream commit since upstream was correct from the get-go. The bot
> should not complain about such patches.
>
> Funnily enough the bot did not complain there even though that patch
> also does not have an upstream commit hash. But it puts
> stable@vger.kernel.org in Cc instead of To.
> 
> [0] https://lore.kernel.org/all/20230411130210.113555-1-ptyadav@amazon.de/
>

Thanks for the information.

As for the patch at [0], the change log has:

This assignment was present in the upstream commit
5891cd5ec46c2 ("net_sched: add __rcu annotation to netdev->qdisc") ...

The robot wrongly considered the phrase "upstream commit ..." as
upstream info. Sorry about this.

We will keep improving the robot to understand various cases, but still
coulnd't avoid sending false positives sometimes. We apologize if the
robot makes any noise. We will fix the robot to correctly handle the
cases discussed in this thread. Thanks.

--
Best Regards,
Yujie