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;
Date:   Wed, 2 Aug 2023 13:25:47 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Su Hui <suhui@nfschina.com>
Cc:     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, ndesaulniers@google.com, 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
Subject: Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter
Message-ID: <531df8ee-ba09-49df-8201-4221df5853c6@kadam.mountain>
References: <20230802080544.3239967-1-suhui@nfschina.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230802080544.3239967-1-suhui@nfschina.com>
Precedence: bulk

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