| Message ID | 20230802080544.3239967-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: lockd: avoid possible wrong NULL parameter | expand |
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
There was a big fight about memcpy() in 2010. https://lwn.net/Articles/416821/ It's sort of related but also sort of different. My understanding is that the glibc memcpy() says that memcpy() always does a dereference so it can delete all the NULL checks which come after. The linux kernel uses -fno-delete-null-pointer-checks to turn this behavior off. regards, dan carpenter
On Wed, Aug 2, 2023 at 3:25 AM 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 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 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
On 2023/8/2 18:41, Dan Carpenter wrote: > There was a big fight about memcpy() in 2010. > > https://lwn.net/Articles/416821/ > > It's sort of related but also sort of different. My understanding is > that the glibc memcpy() says that memcpy() always does a dereference so > it can delete all the NULL checks which come after. The linux kernel > uses -fno-delete-null-pointer-checks to turn this behavior off. Really big fight! This article seems talk about problem that using memcpy() to copy overlapping regions. I'm not sure glibc memcpy does the check about NULL， but glibc printf does this check. "And GNU libc checks strings passed to printf for a %s placeholder for NULL, when the C standard says this is not allowed."[1] [1] https://lwn.net/Articles/416821/ > > regards, > dan carpenter
On 2023/8/3 05:40, Nick Desaulniers wrote: > On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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 > >
I could have sworn there was an issue with glibc annotating these parameters as non-NULL, but maybe it was a different function. With the memcpy() bug in 2010, there were never any numbers to show that it helped improve performance. The only person who measured was Linus and it hurt performance on his laptop. So from a kernel developer perspective it just seemed totally bonkers. 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
On Wed, Aug 2, 2023 at 9:11 PM Su Hui <suhui@nfschina.com> wrote: > > On 2023/8/3 05:40, Nick Desaulniers wrote: > > On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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. Ah, you're right; I agree. What are your thoughts then about moving the "is hostname NULL" check to nsm_create_handle() then? Perhaps nsm_create_handle() should check that immediately and return NULL, or simply skip the memcpy if hostname is NULL? It seems perhaps best to localize this to the callee rather than the caller. While there is only one caller today, should another arise someday, they may make the same mistake. I don't feel strongly either way, and am happy to sign off on either approach; just triple checking we've considered a few different cases. > > 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 } >
On Thu, 2023-08-03 at 09:38 -0700, Nick Desaulniers wrote: > On Wed, Aug 2, 2023 at 9:11 PM Su Hui <suhui@nfschina.com> wrote: > > > > On 2023/8/3 05:40, Nick Desaulniers wrote: > > > > On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@linaro.org> 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. > > Ah, you're right; I agree. > > What are your thoughts then about moving the "is hostname NULL" check > to nsm_create_handle() then? > > Perhaps nsm_create_handle() should check that immediately and return > NULL, or simply skip the memcpy if hostname is NULL? > > It seems perhaps best to localize this to the callee rather than the > caller. While there is only one caller today, should another arise > someday, they may make the same mistake. > > I don't feel strongly either way, and am happy to sign off on either > approach; just triple checking we've considered a few different cases. > I think that sounds like the best fix here. Just have nsm_create_handle return NULL immediately if 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 } > > > >
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); if (unlikely(new == NULL)) return NULL;
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(+)