Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802080544.3239967-1-suhui@nfschina.com> <531df8ee-ba09-49df-8201-4221df5853c6@kadam.mountain>
In-Reply-To: <531df8ee-ba09-49df-8201-4221df5853c6@kadam.mountain>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Wed, 2 Aug 2023 14:40:18 -0700
Message-ID: <CAKwvOdnRwmxGuEidZ=OWxSX60D6ry0Rb__DjSayga6um35Jsrg@mail.gmail.com>
Subject: Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     Su Hui <suhui@nfschina.com>, chuck.lever@oracle.com,
        jlayton@kernel.org, neilb@suse.de, kolga@netapp.com,
        Dai.Ngo@oracle.com, tom@talpey.com,
        trond.myklebust@hammerspace.com, anna@kernel.org,
        nathan@kernel.org, trix@redhat.com, bfields@fieldses.org,
        linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev, kernel-janitors@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 2, 2023 at 3:25=E2=80=AFAM Dan Carpenter <dan.carpenter@linaro.=
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 wil=
l
> > 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@nfschina.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 =3D 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 ove=
rlap, 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) !=3D NULL) {

Which implies that hostname COULD be NULL.

Should this perhaps simply be rewritten as:

if (!hostname)
  return NULL;
if (memchr(...) !=3D 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 protoco=
l,
>    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 =3D {
>    226                  .server         =3D 0,
>    227                  .sap            =3D sap,
>    228                  .salen          =3D salen,
>    229                  .protocol       =3D protocol,
>    230                  .version        =3D version,
>    231                  .hostname       =3D hostname,
>    232                  .hostname_len   =3D strlen(hostname),
>                                                  ^^^^^^^^
> Dereferenced
>
>    233                  .noresvport     =3D noresvport,
>    234                  .net            =3D net,
>    235                  .cred           =3D cred,
>    236          };
>    237          struct hlist_head *chain;
>    238          struct nlm_host *host;
>    239          struct nsm_handle *nsm =3D NULL;
>    240          struct lockd_net *ln =3D net_generic(net, lockd_net_id);
>    241
>    242          dprintk("lockd: %s(host=3D'%s', vers=3D%u, proto=3D%s)\n"=
, __func__,
>    243                          (hostname ? hostname : "<none>"), version=
,
>                                  ^^^^^^^^
> Checked too late.
>
>    244                          (protocol =3D=3D IPPROTO_UDP ? "udp" : "t=
cp"));
>    245
>
> regards,
> dan carpenter


--=20
Thanks,
~Nick Desaulniers