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 <[email protected]>
> ---
> 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
On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <[email protected]> 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 <[email protected]>
> > ---
> > 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
On 2023/8/3 05:40, Nick Desaulniers wrote:
> On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <[email protected]> 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
>
>
On Thu, Aug 03, 2023 at 12:16:40PM +0800, Su Hui wrote:
> Should I send a separate patch for this bug and add you as Reported-by ?
I feel like neither of us know if we should add a check to the
intializer or remove the check from dprintk. If you know the answer,
than absolutely, please send a patch.
regards,
dan carpenter