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]

Message-ID: <CAKwvOdnRwmxGuEidZ=OWxSX60D6ry0Rb__DjSayga6um35Jsrg@mail.gmail.com>
Date:   Wed, 2 Aug 2023 14:40:18 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Su Hui <suhui@...china.com>, 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 Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@...aro.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@...china.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