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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Aug 2023 12:16:40 +0800
From:   Su Hui <suhui@...china.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Dan Carpenter <dan.carpenter@...aro.org>
Cc:     chuck.lever@...cle.com, jlayton@...nel.org, 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 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.

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