[PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Su Hui]

Clang static checker (scan-build) warning:
	cifsglob.h:line 890, column 3
	Access to field 'ops' results in a dereference of a null pointer.

Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
R/W requests") adds a check for 'rdata->server', and let clang throw this
warning about NULL dereference.

When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
add_credits_and_wake_if() will call rdata->server->ops->add_credits().
This will cause NULL dereference problem. Add a check for 'rdata->server'
to avoid NULL dereference.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 fs/smb/client/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index b2405dd4d4d4..45459af5044d 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -315,7 +315,7 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
 #endif
 	}
 
-	if (rdata->credits.value != 0)
+	if (rdata->credits.value != 0) {
 		trace_smb3_rw_credits(rdata->rreq->debug_id,
 				      rdata->subreq.debug_index,
 				      rdata->credits.value,
@@ -323,8 +323,12 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
 				      rdata->server ? rdata->server->in_flight : 0,
 				      -rdata->credits.value,
 				      cifs_trace_rw_credits_free_subreq);
+		if (rdata->server)
+			add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
+		else
+			rdata->credits.value = 0;
+	}
 
-	add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
 	if (rdata->have_xid)
 		free_xid(rdata->xid);
 }
-- 
2.30.2

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Steve French]

Tentatively merged into cifs-2.6.git pending review/testing

Did minor update to add Cc: stable

On Thu, Aug 8, 2024 at 7:26 AM Su Hui <suhui@nfschina.com> wrote:
>
> Clang static checker (scan-build) warning:
>         cifsglob.h:line 890, column 3
>         Access to field 'ops' results in a dereference of a null pointer.
>
> Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> R/W requests") adds a check for 'rdata->server', and let clang throw this
> warning about NULL dereference.
>
> When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> This will cause NULL dereference problem. Add a check for 'rdata->server'
> to avoid NULL dereference.
>
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  fs/smb/client/file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index b2405dd4d4d4..45459af5044d 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -315,7 +315,7 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
>  #endif
>         }
>
> -       if (rdata->credits.value != 0)
> +       if (rdata->credits.value != 0) {
>                 trace_smb3_rw_credits(rdata->rreq->debug_id,
>                                       rdata->subreq.debug_index,
>                                       rdata->credits.value,
> @@ -323,8 +323,12 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
>                                       rdata->server ? rdata->server->in_flight : 0,
>                                       -rdata->credits.value,
>                                       cifs_trace_rw_credits_free_subreq);
> +               if (rdata->server)
> +                       add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
> +               else
> +                       rdata->credits.value = 0;
> +       }
>
> -       add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
>         if (rdata->have_xid)
>                 free_xid(rdata->xid);
>  }
> --
> 2.30.2
>
>


-- 
Thanks,

Steve

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Dan Carpenter]

On Thu, Aug 08, 2024 at 08:23:32PM +0800, Su Hui wrote:
> Clang static checker (scan-build) warning:
> 	cifsglob.h:line 890, column 3
> 	Access to field 'ops' results in a dereference of a null pointer.
> 
> Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> R/W requests") adds a check for 'rdata->server', and let clang throw this
> warning about NULL dereference.
> 
> When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> This will cause NULL dereference problem. Add a check for 'rdata->server'
> to avoid NULL dereference.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>

Needs a Fixes tag.

Also when you add a Fixes tag, then get_maintainer will add the David Howells
automatically.  I've added him manually.

> ---
>  fs/smb/client/file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index b2405dd4d4d4..45459af5044d 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -315,7 +315,7 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
>  #endif
>  	}
>  
> -	if (rdata->credits.value != 0)
> +	if (rdata->credits.value != 0) {
>  		trace_smb3_rw_credits(rdata->rreq->debug_id,
>  				      rdata->subreq.debug_index,
>  				      rdata->credits.value,
> @@ -323,8 +323,12 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
>  				      rdata->server ? rdata->server->in_flight : 0,
>  				      -rdata->credits.value,
>  				      cifs_trace_rw_credits_free_subreq);
> +		if (rdata->server)
> +			add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
> +		else
> +			rdata->credits.value = 0;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^
Why do this?

regards,
dan carpenter

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Dan Carpenter]

On Fri, Aug 09, 2024 at 06:00:26PM +0300, Dan Carpenter wrote:
> On Thu, Aug 08, 2024 at 08:23:32PM +0800, Su Hui wrote:
> > Clang static checker (scan-build) warning:
> > 	cifsglob.h:line 890, column 3
> > 	Access to field 'ops' results in a dereference of a null pointer.
> > 
> > Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> > R/W requests") adds a check for 'rdata->server', and let clang throw this
> > warning about NULL dereference.
> > 
> > When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> > add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> > This will cause NULL dereference problem. Add a check for 'rdata->server'
> > to avoid NULL dereference.
> > 
> > Signed-off-by: Su Hui <suhui@nfschina.com>
> 
> Needs a Fixes tag.
> 
> Also when you add a Fixes tag, then get_maintainer will add the David Howells
> automatically.  I've added him manually.
> 

Actually, David should have been CC'd but the fixes tag wouldn't have pointed
to his patch.

This is an inconsistent NULL checking warning.  It's not clear to me if the NULL
checks should be removed or more added.  If David were trying to fix a NULL
pointer dereference and accidentally left one unchecked dereference out then the
Fixes tag would point to his patch.  Since David was doing something unrelated
then we don't point to his patch.  Instead it would point to the first patch to
introduce the dereference.

regards,
dan carpenter

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Steve French]

On Fri, Aug 9, 2024 at 10:11 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Aug 09, 2024 at 06:00:26PM +0300, Dan Carpenter wrote:
> > On Thu, Aug 08, 2024 at 08:23:32PM +0800, Su Hui wrote:
> > > Clang static checker (scan-build) warning:
> > >     cifsglob.h:line 890, column 3
> > >     Access to field 'ops' results in a dereference of a null pointer.
> > >
> > > Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> > > R/W requests") adds a check for 'rdata->server', and let clang throw this
> > > warning about NULL dereference.
> > >
> > > When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> > > add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> > > This will cause NULL dereference problem. Add a check for 'rdata->server'
> > > to avoid NULL dereference.
> > >
> > > Signed-off-by: Su Hui <suhui@nfschina.com>
> >
> > Needs a Fixes tag.
> >
> > Also when you add a Fixes tag, then get_maintainer will add the David Howells
> > automatically.  I've added him manually.
> >
>
> Actually, David should have been CC'd but the fixes tag wouldn't have pointed
> to his patch.
>
> This is an inconsistent NULL checking warning.  It's not clear to me if the NULL
> checks should be removed or more added.  If David were trying to fix a NULL
> pointer dereference and accidentally left one unchecked dereference out then the
> Fixes tag would point to his patch.  Since David was doing something unrelated

Looks like (if this is even possible for server to to be null) then I
will need to change
the fixes to commit 69c3c023af25. I will update the tag in the current
patch in for-next

Author: David Howells <dhowells@redhat.com>
Date:   Fri Oct 6 18:16:15 2023 +0100

    cifs: Implement netfslib hooks

    Provide implementation of the netfslib hooks that will be used by netfslib
    to ask cifs to set up and perform operations.
-- 
Thanks,

Steve

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Steve French]

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

Updated patch to change Fixes tag and Cc: David


On Fri, Aug 9, 2024 at 10:42 AM Steve French <smfrench@gmail.com> wrote:
>
> On Fri, Aug 9, 2024 at 10:11 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Fri, Aug 09, 2024 at 06:00:26PM +0300, Dan Carpenter wrote:
> > > On Thu, Aug 08, 2024 at 08:23:32PM +0800, Su Hui wrote:
> > > > Clang static checker (scan-build) warning:
> > > >     cifsglob.h:line 890, column 3
> > > >     Access to field 'ops' results in a dereference of a null pointer.
> > > >
> > > > Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> > > > R/W requests") adds a check for 'rdata->server', and let clang throw this
> > > > warning about NULL dereference.
> > > >
> > > > When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> > > > add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> > > > This will cause NULL dereference problem. Add a check for 'rdata->server'
> > > > to avoid NULL dereference.
> > > >
> > > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > >
> > > Needs a Fixes tag.
> > >
> > > Also when you add a Fixes tag, then get_maintainer will add the David Howells
> > > automatically.  I've added him manually.
> > >
> >
> > Actually, David should have been CC'd but the fixes tag wouldn't have pointed
> > to his patch.
> >
> > This is an inconsistent NULL checking warning.  It's not clear to me if the NULL
> > checks should be removed or more added.  If David were trying to fix a NULL
> > pointer dereference and accidentally left one unchecked dereference out then the
> > Fixes tag would point to his patch.  Since David was doing something unrelated
>
> Looks like (if this is even possible for server to to be null) then I
> will need to change
> the fixes to commit 69c3c023af25. I will update the tag in the current
> patch in for-next
>
> Author: David Howells <dhowells@redhat.com>
> Date:   Fri Oct 6 18:16:15 2023 +0100
>
>     cifs: Implement netfslib hooks
>
>     Provide implementation of the netfslib hooks that will be used by netfslib
>     to ask cifs to set up and perform operations.
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb-client-avoid-possible-NULL-dereference-in-cifs_f.patch --]
[-- Type: text/x-patch, Size: 2025 bytes --]

From 59ac1aac1550731ca241007af660fc5278c88136 Mon Sep 17 00:00:00 2001
From: Su Hui <suhui@nfschina.com>
Date: Thu, 8 Aug 2024 20:23:32 +0800
Subject: [PATCH] smb/client: avoid possible NULL dereference in
 cifs_free_subrequest()

Clang static checker (scan-build) warning:
	cifsglob.h:line 890, column 3
	Access to field 'ops' results in a dereference of a null pointer.

Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
R/W requests") adds a check for 'rdata->server', and let clang throw this
warning about NULL dereference.

When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
add_credits_and_wake_if() will call rdata->server->ops->add_credits().
This will cause NULL dereference problem. Add a check for 'rdata->server'
to avoid NULL dereference.

Cc: stable@vger.kernel.org
Fixes: 69c3c023af25 ("cifs: Implement netfslib hooks")
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Su Hui <suhui@nfschina.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index b2405dd4d4d4..45459af5044d 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -315,7 +315,7 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
 #endif
 	}
 
-	if (rdata->credits.value != 0)
+	if (rdata->credits.value != 0) {
 		trace_smb3_rw_credits(rdata->rreq->debug_id,
 				      rdata->subreq.debug_index,
 				      rdata->credits.value,
@@ -323,8 +323,12 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
 				      rdata->server ? rdata->server->in_flight : 0,
 				      -rdata->credits.value,
 				      cifs_trace_rw_credits_free_subreq);
+		if (rdata->server)
+			add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
+		else
+			rdata->credits.value = 0;
+	}
 
-	add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
 	if (rdata->have_xid)
 		free_xid(rdata->xid);
 }
-- 
2.43.0

Re: [PATCH] smb/client: avoid possible NULL dereference in cifs_free_subrequest()

[Steve French]

On Fri, Aug 9, 2024 at 10:01 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Thu, Aug 08, 2024 at 08:23:32PM +0800, Su Hui wrote:
> > Clang static checker (scan-build) warning:
> >       cifsglob.h:line 890, column 3
> >       Access to field 'ops' results in a dereference of a null pointer.
> >
> > Commit 519be989717c ("cifs: Add a tracepoint to track credits involved in
> > R/W requests") adds a check for 'rdata->server', and let clang throw this
> > warning about NULL dereference.
> >
> > When 'rdata->credits.value != 0 && rdata->server == NULL' happens,
> > add_credits_and_wake_if() will call rdata->server->ops->add_credits().
> > This will cause NULL dereference problem. Add a check for 'rdata->server'
> > to avoid NULL dereference.
> >
> > Signed-off-by: Su Hui <suhui@nfschina.com>
>
> Needs a Fixes tag.

I had added a fixes tag

> Also when you add a Fixes tag, then get_maintainer will add the David Howells
> automatically.  I've added him manually.
>
> > ---
> >  fs/smb/client/file.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index b2405dd4d4d4..45459af5044d 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -315,7 +315,7 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
> >  #endif
> >       }
> >
> > -     if (rdata->credits.value != 0)
> > +     if (rdata->credits.value != 0) {
> >               trace_smb3_rw_credits(rdata->rreq->debug_id,
> >                                     rdata->subreq.debug_index,
> >                                     rdata->credits.value,
> > @@ -323,8 +323,12 @@ static void cifs_free_subrequest(struct netfs_io_subrequest *subreq)
> >                                     rdata->server ? rdata->server->in_flight : 0,
> >                                     -rdata->credits.value,
> >                                     cifs_trace_rw_credits_free_subreq);
> > +             if (rdata->server)
> > +                     add_credits_and_wake_if(rdata->server, &rdata->credits, 0);
> > +             else
> > +                     rdata->credits.value = 0;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^
> Why do this?

add_credits_and_wake() has the effect of setting credits->value to 0
so seems reasonable here
(in the case where add_credits_and_wake can not be safely called),
even if an extremely unlikely for rdata->server to be null.





-- 
Thanks,

Steve