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

[Nick Desaulniers]

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