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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 03 Aug 2023 12:56:53 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Su Hui <suhui@...china.com>
Cc:     Dan Carpenter <dan.carpenter@...aro.org>, chuck.lever@...cle.com,
        neilb@...e.de, kolga@...app.com, Dai.Ngo@...cle.com,
        tom@...pey.com, trond.myklebust@...merspace.com, anna@...nel.org,
        nathan@...nel.org, trix@...hat.com, bfields@...ldses.org,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter

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