[bug report] fsinfo: Allow fsinfo() to look up a mount object by ID

[bug report] fsinfo: Allow fsinfo() to look up a mount object by ID

[dan.carpenter]

Hello David Howells,

The patch 2421474bbbc8: "fsinfo: Allow fsinfo() to look up a mount
object by ID" from Jul 5, 2019, leads to the following static checker
warning:

	fs/fsinfo.c:618 vfs_fsinfo_mount()
	warn: AAA no lower bound on 'mnt_id'

fs/fsinfo.c
   589  static int vfs_fsinfo_mount(int dfd, const char __user *filename,
   590                              struct fsinfo_context *ctx)
   591  {
   592          struct path path;
   593          struct fd f = {};
   594          char *name;
   595          long mnt_id;
                ^^^^^^^^^^^

   596          int ret;
   597  
   598          if (!filename)
   599                  return -EINVAL;
   600  
   601          name = strndup_user(filename, 32);
   602          if (IS_ERR(name))
   603                  return PTR_ERR(name);
   604          ret = kstrtoul(name, 0, &mnt_id);
   605          if (ret < 0)
   606                  goto out_name;
   607          if (mnt_id > INT_MAX)
                    ^^^^^^^^^^^^^^^^
This can be negative.  Why do we need to check this at all?  Can we just
delete this check?

   608                  goto out_name;
   609  
   610          if (dfd != AT_FDCWD) {
   611                  ret = -EBADF;
   612                  f = fdget_raw(dfd);
   613                  if (!f.file)
   614                          goto out_name;
   615          }
   616  
   617          ret = lookup_mount_object(f.file ? &f.file->f_path : NULL,
   618                                    mnt_id, &path);
   619          if (ret < 0)
   620                  goto out_fd;
   621  
   622          ret = vfs_fsinfo(&path, ctx);
   623          path_put(&path);
   624  out_fd:
   625          fdput(f);
   626  out_name:
   627          kfree(name);
   628          return ret;
   629  }

regards,
dan carpenter

Re: [bug report] fsinfo: Allow fsinfo() to look up a mount object by ID

[David Howells]

<dan.carpenter@oracle.com> wrote:

>    604          ret = kstrtoul(name, 0, &mnt_id);

Hmmm...  I'm a bit surprised there isn't a warning generated: kstrtoul() takes
a pointer to an unsigned long, not a long.  mnt_id should be an unsigned
long.  I'll fix that.

>    607          if (mnt_id > INT_MAX)
>                     ^^^^^^^^^^^^^^^^
> This can be negative.  Why do we need to check this at all?  Can we just
> delete this check?

As we get a long, it can go out of range for the mount parameter we're
checking - which is an int.  I feel it's better to restrict it so that we
don't get a false match due to implicit masking.

I wonder if struct mount::mnt_id should actually be unsigned int rather than
int - there doesn't seem any reason it should go negative.

David