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