On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@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 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
