From: Linus Torvalds <torvalds@linux-foundation.org>
To: Guenter Roeck <linux@roeck-us.net>,
Yoshinori Sato <ysato@users.sourceforge.jp>,
Rich Felker <dalias@libc.org>,
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Cc: Miklos Szeredi <mszeredi@redhat.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-api@vger.kernel.org, linux-man@vger.kernel.org,
linux-security-module@vger.kernel.org,
Karel Zak <kzak@redhat.com>, Ian Kent <raven@themaw.net>,
David Howells <dhowells@redhat.com>,
Al Viro <viro@zeniv.linux.org.uk>,
Christian Brauner <christian@brauner.io>,
Amir Goldstein <amir73il@gmail.com>,
Matthew House <mattlloydhouse@gmail.com>,
Florian Weimer <fweimer@redhat.com>,
Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH v4 5/6] add listmount(2) syscall
Date: Thu, 11 Jan 2024 12:14:07 -0800 [thread overview]
Message-ID: <CAHk-=wjh6Cypo8WC-McXgSzCaou3UXccxB+7PVeSuGR8AjCphg@mail.gmail.com> (raw)
In-Reply-To: <2f595f28-7fcd-4196-a0b1-6598781530b9@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]
On Thu, 11 Jan 2024 at 10:57, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Any variance of put_user() with &buf[ctr] or buf + ctr fails
> if ctr is a variable and permitted to be != 0.
Crazy. But the 64-bit put_user() is a bit special and tends to require
more registers (the 64-bit value is passed in two registers), so that
probably then results in the ICE.
Side note: looking at the SH version of __put_user_u64(), I think it's
buggy and is missing the exception handler for the second 32-bit move.
I dunno, I don't read sh asm, but it looks suspicious.
> The following works. Would this be acceptable ?
It might be very easy to trigger this once again if somebody goes "that's silly"
That said, I also absolutely detest the "error handling" in that
function. It's horrible.
Noticing the user access error in the middle is just sad, and if that
was just handled better and at least the range was checked first, the
overflow error couldn't happen and checking for it is thus pointless.
And looking at it all, it really looks like the whole interface is
broken. The "bufsize" argument isn't the size of the buffer at all.
It's the number of entries.
Extra confusingly, in the *other* system call, bufsize is in fact the
size of the buffer.
And the 'ctr' overflow checking is doubly garbage, because the only
reason *that* can happen is that we didn't check the incoming
arguments properly.
Same goes for the whole array_index_nospec() - it's pointless, because
the user controls what that code checks against anyway, so there's no
point to trying to manage some range checking.
The only range checking there that matters would be the one that
put_user() has to do against the address space size, but that's done
by put_user().
End result: that thing needs a rewrite.
The SH put_user64() needs to be looked at too, but in the meantime,
maybe something like this fixes the problems with listmount?
NOTE! ENTIRELY untested, but that naming and lack of argument sanity
checking really is horrendous. We should have caught this earlier.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2265 bytes --]
fs/namespace.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index ef1fd6829814..df74f4769733 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5043,12 +5043,17 @@ static struct mount *listmnt_next(struct mount *curr)
}
static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
- u64 __user *buf, size_t bufsize,
+ u64 __user *buf, size_t nentries,
const struct path *root)
{
struct mount *r;
- ssize_t ctr;
- int err;
+ const size_t maxentries = (size_t)-1 >> 3;
+ ssize_t ret;
+
+ if (unlikely(nentries > maxentries))
+ return -EFAULT;
+ if (!access_ok(buf, nentries * sizeof(*buf)))
+ return -EFAULT;
/*
* Don't trigger audit denials. We just want to determine what
@@ -5058,26 +5063,24 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
!ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN))
return -EPERM;
- err = security_sb_statfs(orig->dentry);
- if (err)
- return err;
+ ret = security_sb_statfs(orig->dentry);
+ if (ret)
+ return ret;
- for (ctr = 0, r = first; r && ctr < bufsize; r = listmnt_next(r)) {
+ for (ret = 0, r = first; r && nentries; r = listmnt_next(r)) {
if (r->mnt_id_unique == mnt_id)
continue;
if (!is_path_reachable(r, r->mnt.mnt_root, orig))
continue;
- ctr = array_index_nospec(ctr, bufsize);
- if (put_user(r->mnt_id_unique, buf + ctr))
+ if (put_user(r->mnt_id_unique, buf))
return -EFAULT;
- if (check_add_overflow(ctr, 1, &ctr))
- return -ERANGE;
+ buf++, ret++; nentries--;
}
- return ctr;
+ return ret;
}
SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
- u64 __user *, buf, size_t, bufsize, unsigned int, flags)
+ u64 __user *, buf, size_t, nentries, unsigned int, flags)
{
struct mnt_namespace *ns = current->nsproxy->mnt_ns;
struct mnt_id_req kreq;
@@ -5111,7 +5114,7 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
else
first = mnt_find_id_at(ns, last_mnt_id + 1);
- ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root);
+ ret = do_listmount(first, &orig, mnt_id, buf, nentries, &root);
err:
path_put(&root);
up_read(&namespace_sem);
next prev parent reply other threads:[~2024-01-11 20:14 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-25 14:01 [PATCH v4 0/6] querying mount attributes Miklos Szeredi
2023-10-25 14:01 ` [PATCH v4 1/6] add unique mount ID Miklos Szeredi
2023-10-25 14:02 ` [PATCH v4 2/6] mounts: keep list of mounts in an rbtree Miklos Szeredi
2023-10-27 3:11 ` Ian Kent
2023-10-27 8:17 ` Miklos Szeredi
2023-10-28 1:36 ` Ian Kent
2023-10-30 5:37 ` Ian Kent
2023-10-30 5:45 ` Ian Kent
2023-10-30 9:06 ` Miklos Szeredi
2023-10-31 1:23 ` Ian Kent
2023-10-25 14:02 ` [PATCH v4 3/6] namespace: extract show_path() helper Miklos Szeredi
2023-10-25 14:02 ` [PATCH v4 4/6] add statmount(2) syscall Miklos Szeredi
2023-11-08 2:58 ` Paul Moore
2023-11-08 7:58 ` Christian Brauner
2023-11-08 20:10 ` Paul Moore
2023-11-10 17:00 ` Paul Moore
2023-11-12 13:05 ` Christian Brauner
2023-11-12 20:29 ` Paul Moore
2023-10-25 14:02 ` [PATCH v4 5/6] add listmount(2) syscall Miklos Szeredi
2023-11-07 21:23 ` Jonathan Corbet
2023-11-08 7:53 ` Christian Brauner
2023-11-08 16:20 ` Jonathan Corbet
2023-11-08 16:23 ` Christian Brauner
2023-11-08 2:58 ` Paul Moore
2024-01-10 22:23 ` Guenter Roeck
2024-01-11 0:32 ` Linus Torvalds
2024-01-11 5:12 ` Guenter Roeck
2024-01-11 18:57 ` Guenter Roeck
2024-01-11 20:14 ` Linus Torvalds [this message]
2024-01-11 23:01 ` Arnd Bergmann
2024-01-11 23:57 ` Guenter Roeck
2024-01-12 3:40 ` Linus Torvalds
2024-01-12 5:24 ` Guenter Roeck
2024-01-12 9:00 ` Christian Brauner
2024-01-23 14:14 ` John Paul Adrian Glaubitz
2024-01-23 15:31 ` Guenter Roeck
2024-01-23 14:14 ` John Paul Adrian Glaubitz
2023-10-25 14:02 ` [PATCH v4 6/6] wire up syscalls for statmount/listmount Miklos Szeredi
2024-01-09 1:11 ` Florian Fainelli
2023-11-01 11:13 ` [PATCH v4 0/6] querying mount attributes Christian Brauner
2023-11-01 13:18 ` Miklos Szeredi
2023-11-01 15:54 ` Christian Brauner
2023-11-01 11:52 ` Ian Kent
2023-11-06 12:10 ` Karel Zak
2023-11-06 13:33 ` Amir Goldstein
2023-11-07 0:47 ` Ian Kent
2023-11-06 23:54 ` Ian Kent
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk-=wjh6Cypo8WC-McXgSzCaou3UXccxB+7PVeSuGR8AjCphg@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=amir73il@gmail.com \
--cc=arnd@arndb.de \
--cc=christian@brauner.io \
--cc=dalias@libc.org \
--cc=dhowells@redhat.com \
--cc=fweimer@redhat.com \
--cc=glaubitz@physik.fu-berlin.de \
--cc=kzak@redhat.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-man@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mattlloydhouse@gmail.com \
--cc=mszeredi@redhat.com \
--cc=raven@themaw.net \
--cc=viro@zeniv.linux.org.uk \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).