* [bug report] fsinfo: Allow fsinfo() to look up a mount object by ID
@ 2020-07-13 10:12 dan.carpenter
2020-07-17 17:31 ` David Howells
0 siblings, 1 reply; 2+ messages in thread
From: dan.carpenter @ 2020-07-13 10:12 UTC (permalink / raw
To: dhowells; +Cc: linux-fsdevel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] fsinfo: Allow fsinfo() to look up a mount object by ID
2020-07-13 10:12 [bug report] fsinfo: Allow fsinfo() to look up a mount object by ID dan.carpenter
@ 2020-07-17 17:31 ` David Howells
0 siblings, 0 replies; 2+ messages in thread
From: David Howells @ 2020-07-17 17:31 UTC (permalink / raw
To: dan.carpenter; +Cc: dhowells, linux-fsdevel
<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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-17 17:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-13 10:12 [bug report] fsinfo: Allow fsinfo() to look up a mount object by ID dan.carpenter
2020-07-17 17:31 ` David Howells
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.