From: "Arnd Bergmann" <arnd@arndb.de>
To: "Günther Noack" <gnoack@google.com>
Cc: linux-security-module@vger.kernel.org,
"Mickaël Salaün" <mic@digikod.net>,
"Christian Brauner" <brauner@kernel.org>,
"Jeff Xu" <jeffxu@google.com>,
"Jorge Lucangeli Obes" <jorgelo@chromium.org>,
"Allen Webb" <allenwebb@google.com>,
"Dmitry Torokhov" <dtor@google.com>,
"Paul Moore" <paul@paul-moore.com>,
"Konstantin Meskhidze" <konstantin.meskhidze@huawei.com>,
"Matt Bobrowski" <repnop@google.com>,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v9 1/8] landlock: Add IOCTL access right
Date: Sat, 10 Feb 2024 12:49:23 +0100 [thread overview]
Message-ID: <036db535-587a-4e1b-bd44-345af3b51ddf@app.fastmail.com> (raw)
In-Reply-To: <ZcdYrJfhiNEtqIEW@google.com>
On Sat, Feb 10, 2024, at 12:06, Günther Noack wrote:
> On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
>
> The IOCTL command in question is FIONREAD: fs/ioctl.c implements
> FIONREAD directly for S_ISREG files, but it does call the FIONREAD
> implementation in the VFS layer for other file types.
>
> The question we are asking ourselves is:
>
> * Can we let processes safely use FIONREAD for all files which get
> opened for the purpose of reading, or do we run the risk of
> accidentally exposing surprising IOCTL implementations which have
> completely different purposes?
>
> Is it safe to assume that the VFS layer FIONREAD implementations are
> actually implementing FIONREAD semantics?
>
> * I know there have been accidental collisions of IOCTL command
> numbers in the past -- Hypothetically, if this were to happen in one
> of the VFS implementations of FIONREAD, would that be considered a
> bug that would need to get fixed in that implementation?
Clearly it's impossible to be sure no driver has a conflict
on this particular ioctl, but the risk for one intentionally
overriding it should be fairly low.
There are a couple of possible issues I can think of:
- the numeric value of FIONREAD is different depending
on the architecture, with at least four different numbers
aliasing to it. This is probably harmless but makes it
harder to look for accidental conflicts.
- Aside from FIONREAD, it is sometimes called SIOCINQ
(for sockets) or TIOCINQ (for tty). These still go
through the same VFS entry point and as far as I can
tell always have the same semantics (writing 4 bytes
of data with the count of the remaining bytes in the
fd).
- There are probably a couple of drivers that do something
in their ioctl handler without actually looking at
the command number.
If you want to be really sure you get this right, you
could add a new callback to struct file_operations
that handles this for all drivers, something like
static int ioctl_fionread(struct file *filp, int __user *arg)
{
int n;
if (S_ISREG(inode->i_mode))
return put_user(i_size_read(inode) - filp->f_pos, arg);
if (!file->f_op->fionread)
return -ENOIOCTLCMD;
n = file->f_op->fionread(filp);
if (n < 0)
return n;
return put_user(n, arg);
}
With this, you can go through any driver implementing
FIONREAD/SIOCINQ/TIOCINQ and move the code from .ioctl
into .fionread. This probably results in cleaner code
overall, especially in drivers that have no other ioctl
commands besides this one.
Since sockets and ttys tend to have both SIOCINQ/TIOCINQ
and SIOCOUTQ/TIOCOUTQ (unlike regular files), it's
probably best to do both at the same time, or maybe
have a single callback pointer with an in/out flag.
Arnd
next prev parent reply other threads:[~2024-02-10 11:50 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-09 17:06 [PATCH v9 0/8] Landlock: IOCTL support Günther Noack
2024-02-09 17:06 ` [PATCH v9 1/8] landlock: Add IOCTL access right Günther Noack
2024-02-10 11:06 ` Günther Noack
2024-02-10 11:49 ` Arnd Bergmann [this message]
2024-02-12 11:09 ` Christian Brauner
2024-02-12 22:10 ` Günther Noack
2024-02-10 11:18 ` Günther Noack
2024-02-16 14:11 ` Mickaël Salaün
2024-02-16 15:51 ` Mickaël Salaün
2024-02-18 8:34 ` Günther Noack
2024-02-19 21:44 ` Günther Noack
2024-02-16 17:19 ` Mickaël Salaün
2024-02-19 18:34 ` Mickaël Salaün
2024-02-19 18:35 ` [RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers Mickaël Salaün
2024-03-01 13:42 ` Mickaël Salaün
2024-03-01 16:24 ` Arnd Bergmann
2024-03-01 18:35 ` Mickaël Salaün
2024-03-05 18:13 ` Günther Noack
2024-03-06 13:47 ` Mickaël Salaün
2024-03-06 15:18 ` Arnd Bergmann
2024-03-07 12:15 ` Christian Brauner
2024-03-07 12:21 ` Arnd Bergmann
2024-03-07 12:57 ` Günther Noack
2024-03-07 20:40 ` Paul Moore
2024-03-07 23:09 ` Dave Chinner
2024-03-07 23:35 ` Paul Moore
2024-03-08 7:02 ` Arnd Bergmann
2024-03-08 9:29 ` Mickaël Salaün
2024-03-08 19:22 ` Paul Moore
2024-03-08 20:12 ` Mickaël Salaün
2024-03-08 22:04 ` Casey Schaufler
2024-03-08 22:25 ` Paul Moore
2024-03-09 8:14 ` Günther Noack
2024-03-09 17:41 ` Casey Schaufler
2024-03-11 19:04 ` Paul Moore
2024-03-08 11:03 ` Günther Noack
2024-03-11 1:03 ` Dave Chinner
2024-03-11 9:01 ` Günther Noack
2024-03-11 22:12 ` Dave Chinner
2024-03-12 10:58 ` Mickaël Salaün
2024-02-28 12:57 ` [PATCH v9 1/8] landlock: Add IOCTL access right Günther Noack
2024-03-01 12:59 ` Mickaël Salaün
2024-03-01 13:38 ` Mickaël Salaün
2024-02-09 17:06 ` [PATCH v9 2/8] selftests/landlock: Test IOCTL support Günther Noack
2024-02-09 17:06 ` [PATCH v9 3/8] selftests/landlock: Test IOCTL with memfds Günther Noack
2024-02-09 17:06 ` [PATCH v9 4/8] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH) Günther Noack
2024-02-09 17:06 ` [PATCH v9 5/8] selftests/landlock: Test IOCTLs on named pipes Günther Noack
2024-02-09 17:06 ` [PATCH v9 6/8] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets Günther Noack
2024-02-09 17:06 ` [PATCH v9 7/8] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL Günther Noack
2024-02-09 17:06 ` [PATCH v9 8/8] landlock: Document IOCTL support Günther Noack
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=036db535-587a-4e1b-bd44-345af3b51ddf@app.fastmail.com \
--to=arnd@arndb.de \
--cc=allenwebb@google.com \
--cc=brauner@kernel.org \
--cc=dtor@google.com \
--cc=gnoack@google.com \
--cc=jeffxu@google.com \
--cc=jorgelo@chromium.org \
--cc=konstantin.meskhidze@huawei.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mic@digikod.net \
--cc=paul@paul-moore.com \
--cc=repnop@google.com \
/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).