| Message ID | 20240808122331.342473-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb/client: avoid possible NULL dereference in cifs_free_subrequest() | expand |
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 > >
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
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
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.
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.
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
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); }
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(-)