clang's static analysis warning: fs/lockd/mon.c: line 293, column 2:
Null pointer passed as 2nd argument to memory copy function.
Assuming 'hostname' is NULL and calling 'nsm_create_handle()', this will
pass NULL as 2nd argument to memory copy function 'memcpy()'. So return
NULL if 'hostname' is invalid.
Fixes: 77a3ef33e2de ("NSM: More clean up of nsm_get_handle()")
Signed-off-by: Su Hui <[email protected]>
---
fs/lockd/mon.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 1d9488cf0534..eebab013e063 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -358,6 +358,9 @@ struct nsm_handle *nsm_get_handle(const struct net *net,
spin_unlock(&nsm_lock);
+ if (!hostname)
+ return NULL;
+
new = nsm_create_handle(sap, salen, hostname, hostname_len);
if (unlikely(new == NULL))
return NULL;
--
2.30.2
There was a big fight about memcpy() in 2010.
https://lwn.net/Articles/416821/
It's sort of related but also sort of different. My understanding is
that the glibc memcpy() says that memcpy() always does a dereference so
it can delete all the NULL checks which come after. The linux kernel
uses -fno-delete-null-pointer-checks to turn this behavior off.
regards,
dan carpenter
I could have sworn there was an issue with glibc annotating these
parameters as non-NULL, but maybe it was a different function.
With the memcpy() bug in 2010, there were never any numbers to show that
it helped improve performance. The only person who measured was Linus
and it hurt performance on his laptop. So from a kernel developer
perspective it just seemed totally bonkers.
regards,
dan carpenter
On Thu, 2023-08-03 at 09:38 -0700, Nick Desaulniers wrote:
> On Wed, Aug 2, 2023 at 9:11 PM Su Hui <[email protected]> wrote:
> >
> > On 2023/8/3 05:40, Nick Desaulniers wrote:
> >
> > On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <[email protected]> wrote:
> >
> >
> > I noticed that the function in question already has a guard:
> > 322 if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
> >
> > Which implies that hostname COULD be NULL.
> >
> > Should this perhaps simply be rewritten as:
> >
> > if (!hostname)
> > return NULL;
> > if (memchr(...) != NULL)
> > ...
> >
> > Rather than bury yet another guard for the same check further down in
> > the function? Check once and bail early.
> >
> > Hi, Nick Desaulnier,
> >
> > This may disrupt the logic of this function. So maybe the past one is better.
> >
> > 322 if (!hostname)
> > 323 return NULL;
> > ^^^^^^^^^^^^
> > If we return in this place.
> >
> > 324 if (memchr(hostname, '/', hostname_len) != NULL) {
> > 325 if (printk_ratelimit()) {
> > 326 printk(KERN_WARNING "Invalid hostname \"%.*s\" "
> > 327 "in NFS lock request\n",
> > 328 (int)hostname_len, hostname);
> > 329 }
> > 330 return NULL;
> > 331 }
> > 332
> > 333 retry:
> > 334 spin_lock(&nsm_lock);
> > 335
> > 336 if (nsm_use_hostnames && hostname != NULL)
> > 337 cached = nsm_lookup_hostname(&ln->nsm_handles,
> > 338 hostname, hostname_len);
> > 339 else
> > 340 cached = nsm_lookup_addr(&ln->nsm_handles, sap);
> > ^^^^^^^^^^^^^^^
> > This case will be broken when hostname is NULL.
>
> Ah, you're right; I agree.
>
> What are your thoughts then about moving the "is hostname NULL" check
> to nsm_create_handle() then?
>
> Perhaps nsm_create_handle() should check that immediately and return
> NULL, or simply skip the memcpy if hostname is NULL?
>
> It seems perhaps best to localize this to the callee rather than the
> caller. While there is only one caller today, should another arise
> someday, they may make the same mistake.
>
> I don't feel strongly either way, and am happy to sign off on either
> approach; just triple checking we've considered a few different cases.
>
I think that sounds like the best fix here. Just have nsm_create_handle
return NULL immediately if hostname is NULL.
> >
> > 341
> > 342 if (cached != NULL) {
> > 343 refcount_inc(&cached->sm_count);
> > 344 spin_unlock(&nsm_lock);
> > 345 kfree(new);
> > 346 dprintk("lockd: found nsm_handle for %s (%s), "
> > 347 "cnt %d\n", cached->sm_name,
> > 348 cached->sm_addrbuf,
> > 349 refcount_read(&cached->sm_count));
> > 350 return cached;
> > 351 }
> >
>
>
--
Jeff Layton <[email protected]>