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@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.
> 

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@xxxxxxxxxx>