[PATCH] fs: lockd: avoid possible wrong NULL parameter

[PATCH] fs: lockd: avoid possible wrong NULL parameter

[Su Hui]

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 <suhui@nfschina.com>
---
 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

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Dan Carpenter]

On Wed, Aug 02, 2023 at 04:05:45PM +0800, Su Hui wrote:
> 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 <suhui@nfschina.com>
> ---
>  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);

It's weird that this bug is from 2008 and we haven't found it in
testing.  Presumably if hostname is NULL then hostname_len would be zero
and in that case, it's not actually a bug.  It's allowed in the kernel
to memcpy zero bytes from a NULL pointer.

	memcpy(dst, NULL, 0);

Outside the kernel it's not allowed though.

I noticed a related bug which Smatch doesn't find, because of how Smatch
handles the dprintk macro.

fs/lockd/host.c
truct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
   217                                       const size_t salen,
   218                                       const unsigned short protocol,
   219                                       const u32 version,
   220                                       const char *hostname,
   221                                       int noresvport,
   222                                       struct net *net,
   223                                       const struct cred *cred)
   224  {
   225          struct nlm_lookup_host_info ni = {
   226                  .server         = 0,
   227                  .sap            = sap,
   228                  .salen          = salen,
   229                  .protocol       = protocol,
   230                  .version        = version,
   231                  .hostname       = hostname,
   232                  .hostname_len   = strlen(hostname),
                                                 ^^^^^^^^
Dereferenced

   233                  .noresvport     = noresvport,
   234                  .net            = net,
   235                  .cred           = cred,
   236          };
   237          struct hlist_head *chain;
   238          struct nlm_host *host;
   239          struct nsm_handle *nsm = NULL;
   240          struct lockd_net *ln = net_generic(net, lockd_net_id);
   241  
   242          dprintk("lockd: %s(host='%s', vers=%u, proto=%s)\n", __func__,
   243                          (hostname ? hostname : "<none>"), version,
                                 ^^^^^^^^
Checked too late.

   244                          (protocol == IPPROTO_UDP ? "udp" : "tcp"));
   245  

regards,
dan carpenter

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Dan Carpenter]

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

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Su Hui]

On 2023/8/2 18:41, Dan Carpenter wrote:
> 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.
Really big fight!
This article seems talk about  problem that using memcpy() to copy 
overlapping regions.
I'm not sure glibc memcpy does the check about NULL， but glibc printf 
does this check.
"And GNU libc checks strings passed to printf for a %s placeholder for 
NULL,
when the C standard says this is not allowed."[1]
[1] https://lwn.net/Articles/416821/
>
> regards,
> dan carpenter

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Nick Desaulniers]

On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Aug 02, 2023 at 04:05:45PM +0800, Su Hui wrote:
> > 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 <suhui@nfschina.com>
> > ---
> >  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);
>
> It's weird that this bug is from 2008 and we haven't found it in
> testing.  Presumably if hostname is NULL then hostname_len would be zero
> and in that case, it's not actually a bug.  It's allowed in the kernel
> to memcpy zero bytes from a NULL pointer.
>
>         memcpy(dst, NULL, 0);
>
> Outside the kernel it's not allowed though.

I wonder what kind of implications that has on the compilers ability
to optimize libcalls to memcpy for targets that don't use
`-ffreestanding`. Hmm...

Though let's see what the C standard says, since that's what compilers
target, rather than consider specifics of glibc.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
>> The memcpy function copies n characters from the object pointed to by s2 into the
>> object pointed to by s1. If copying takes place between objects that overlap, the behavior
>> is undefined.

So no mention about what assumptions can be made about source or
destination being NULL.

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.

>
> I noticed a related bug which Smatch doesn't find, because of how Smatch
> handles the dprintk macro.
>
> fs/lockd/host.c
> truct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
>    217                                       const size_t salen,
>    218                                       const unsigned short protocol,
>    219                                       const u32 version,
>    220                                       const char *hostname,
>    221                                       int noresvport,
>    222                                       struct net *net,
>    223                                       const struct cred *cred)
>    224  {
>    225          struct nlm_lookup_host_info ni = {
>    226                  .server         = 0,
>    227                  .sap            = sap,
>    228                  .salen          = salen,
>    229                  .protocol       = protocol,
>    230                  .version        = version,
>    231                  .hostname       = hostname,
>    232                  .hostname_len   = strlen(hostname),
>                                                  ^^^^^^^^
> Dereferenced
>
>    233                  .noresvport     = noresvport,
>    234                  .net            = net,
>    235                  .cred           = cred,
>    236          };
>    237          struct hlist_head *chain;
>    238          struct nlm_host *host;
>    239          struct nsm_handle *nsm = NULL;
>    240          struct lockd_net *ln = net_generic(net, lockd_net_id);
>    241
>    242          dprintk("lockd: %s(host='%s', vers=%u, proto=%s)\n", __func__,
>    243                          (hostname ? hostname : "<none>"), version,
>                                  ^^^^^^^^
> Checked too late.
>
>    244                          (protocol == IPPROTO_UDP ? "udp" : "tcp"));
>    245
>
> regards,
> dan carpenter



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Su Hui]

On 2023/8/3 05:40, Nick Desaulniers wrote:
> On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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.

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         }

>> I noticed a related bug which Smatch doesn't find, because of how Smatch
>> handles the dprintk macro.

Hi, Dan,

Great eye!
Should I send a separate patch for this bug and add you as Reported-by ?

>>
>> fs/lockd/host.c
>> truct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
>>     217                                       const size_t salen,
>>     218                                       const unsigned short protocol,
>>     219                                       const u32 version,
>>     220                                       const char *hostname,
>>     221                                       int noresvport,
>>     222                                       struct net *net,
>>     223                                       const struct cred *cred)
>>     224  {
>>     225          struct nlm_lookup_host_info ni = {
>>     226                  .server         = 0,
>>     227                  .sap            = sap,
>>     228                  .salen          = salen,
>>     229                  .protocol       = protocol,
>>     230                  .version        = version,
>>     231                  .hostname       = hostname,
>>     232                  .hostname_len   = strlen(hostname),
>>                                                   ^^^^^^^^
>> Dereferenced
>>
>>     233                  .noresvport     = noresvport,
>>     234                  .net            = net,
>>     235                  .cred           = cred,
>>     236          };
>>     237          struct hlist_head *chain;
>>     238          struct nlm_host *host;
>>     239          struct nsm_handle *nsm = NULL;
>>     240          struct lockd_net *ln = net_generic(net, lockd_net_id);
>>     241
>>     242          dprintk("lockd: %s(host='%s', vers=%u, proto=%s)\n", __func__,
>>     243                          (hostname ? hostname : "<none>"), version,
>>                                   ^^^^^^^^
>> Checked too late.
>>
>>     244                          (protocol == IPPROTO_UDP ? "udp" : "tcp"));
>>     245
>>
>> regards,
>> dan carpenter
>
>

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Dan Carpenter]

On Thu, Aug 03, 2023 at 12:16:40PM +0800, Su Hui wrote:
> Should I send a separate patch for this bug and add you as Reported-by ?

I feel like neither of us know if we should add a check to the
intializer or remove the check from dprintk.  If you know the answer,
than absolutely, please send a patch.

regards,
dan carpenter

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[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

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Nick Desaulniers]

On Wed, Aug 2, 2023 at 9:11 PM Su Hui <suhui@nfschina.com> wrote:
>
> On 2023/8/3 05:40, Nick Desaulniers wrote:
>
> On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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.

>
> 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         }
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

[Jeff Layton]

On Thu, 2023-08-03 at 09:38 -0700, Nick Desaulniers wrote:
> On Wed, Aug 2, 2023 at 9:11 PM Su Hui <suhui@nfschina.com> wrote:
> > 
> > On 2023/8/3 05:40, Nick Desaulniers wrote:
> > 
> > On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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 <jlayton@kernel.org>