From: "Mickaël Salaün" <mic@digikod.net>
To: "Günther Noack" <gnoack@google.com>
Cc: linux-security-module@vger.kernel.org,
Jeff Xu <jeffxu@google.com>, Arnd Bergmann <arnd@arndb.de>,
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,
Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v13 01/10] landlock: Add IOCTL access right for character and block devices
Date: Fri, 5 Apr 2024 20:01:31 +0200 [thread overview]
Message-ID: <20240405.oPhaosiel1ni@digikod.net> (raw)
In-Reply-To: <ZhAkDW2u3GItsody@google.com>
On Fri, Apr 05, 2024 at 06:17:17PM +0200, Günther Noack wrote:
> On Wed, Apr 03, 2024 at 01:15:45PM +0200, Mickaël Salaün wrote:
> > On Tue, Apr 02, 2024 at 08:28:49PM +0200, Günther Noack wrote:
> > > On Wed, Mar 27, 2024 at 05:57:31PM +0100, Mickaël Salaün wrote:
> > > > On Wed, Mar 27, 2024 at 01:10:31PM +0000, Günther Noack wrote:
> > > > > + case FIOQSIZE:
> > > > > + /*
> > > > > + * FIOQSIZE queries the size of a regular file or directory.
> > > > > + *
> > > > > + * This IOCTL command only applies to regular files and
> > > > > + * directories.
> > > > > + */
> > > > > + return LANDLOCK_ACCESS_FS_IOCTL_DEV;
> > > >
> > > > This should always be allowed because do_vfs_ioctl() never returns
> > > > -ENOIOCTLCMD for this command. That's why I wrote
> > > > vfs_masked_device_ioctl() this way [1]. I think it would be easier to
> > > > read and maintain this code with a is_masked_device_ioctl() logic. Listing
> > > > commands that are not masked makes it difficult to review because
> > > > allowed and denied return codes are interleaved.
> > >
> > > Oh, I misunderstood you on [2], I think -- I was under the impression that you
> > > wanted to keep the switch case in the same order (and with the same entries?) as
> > > the original in do_vfs_ioctl. So you'd prefer to only list the always-allowed
> > > IOCTL commands here, as you have done in vfs_masked_device_ioctl() [3]?
> > >
> > > [2] https://lore.kernel.org/all/20240326.ooCheem1biV2@digikod.net/
> > > [3] https://lore.kernel.org/all/20240219183539.2926165-1-mic@digikod.net/
> >
> > That was indeed unclear. About IOCTL commands, the same order ease
> > reviewing and maintenance but we don't need to list all commands,
> > which will limit updates of this list. However, for the current
> > unused/unmasked one, we can still add them very briefly in comments as I
> > did with FIONREAD and file_ioctl()'s ones in vfs_masked_device_ioctl().
> > Only listing the "masked" ones (for device case) shorten the list, and
> > having a list with the same semantic ("mask device-specific IOCTLs")
> > ease review and maintenance as well.
> >
> > >
> > > Can you please clarify how you make up your mind about what should be permitted
> > > and what should not? I have trouble understanding the rationale for the changes
> > > that you asked for below, apart from the points that they are harmless and that
> > > the return codes should be consistent.
> >
> > The rationale is the same: all IOCTL commands that are not
> > passed/specific to character or block devices (i.e. IOCTLs defined in
> > fs/ioctl.c) are allowed. vfs_masked_device_ioctl() returns true if the
> > IOCTL command is not passed to the related device driver but handled by
> > fs/ioctl.c instead (i.e. handled by the VFS layer).
>
> Thanks for clarifying -- this makes more sense now. I traced the cases with
> -ENOIOCTLCMD through the code more thoroughly and it is more aligned now with
> what you implemented before. The places where I ended up implementing it
> differently to your vfs_masked_device_ioctl() patch are:
>
> * Do not blanket-permit FS_IOC_{GET,SET}{FLAGS,XATTR}.
> They fall back to the device implementation.
>
> * FS_IOC_GETUUID and FS_IOC_GETFSSYSFSPATH are now handled.
> These return -ENOIOCTLCMD from do_vfs_ioctl(), so they do fall back to the
> handlers in struct file_operations, so we can not permit these either.
Good catch!
>
> These seem like pretty clear cases to me.
>
>
> > > The criteria that I have used in this patch set are that (a) it is implemented
> > > in do_vfs_ioctl() rather than further below, and (b) it makes sense to use that
> > > command on a device file. (If we permit FIOQSIZE, FS_IOC_FIEMAP and others
> > > here, we will get slightly more correct error codes in these cases, but the
> > > IOCTLs will still not work, because they are not useful and not implemented for
> > > devices. -- On the other hand, we are also increasing the exposed code surface a
> > > bit. For example, FS_IOC_FIEMAP is calling into inode->i_op->fiemap(). That is
> > > probably harmless for device files, but requires us to reason at a deeper level
> > > to convince ourselves of that.)
> >
> > FIOQSIZE is fully handled by do_vfs_ioctl(), and FS_IOC_FIEMAP is
> > implemented as the inode level, so it should not be passed at the struct
> > file/device level unless ENOIOCTLCMD is returned (but it should not,
> > right?). Because it depends on the inode implementation, it looks like
> > this IOCTL may work (in theory) on character or block devices too. If
> > this is correct, we should not deny it because the semantic of
> > LANDLOCK_ACCESS_FS_IOCTL_DEV is to control IOCTLs passed to device
> > drivers. Furthermore, as you pointed out, error codes would be
> > unaltered.
> >
> > It would be good to test (as you suggested IIRC) the masked commands on
> > a simple device (e.g. /dev/null) to check that it returns ENOTTY,
> > EOPNOTSUPP, or EACCES according to our expectations.
>
> Sounds good, I'll add a test.
>
>
> > I agree that this would increase a bit the exposed code surface but I'm
> > pretty sure that if a sandboxed process is allowed to access a device
> > file, it is also allowed to access directory or other file types as well
> > and then would still be able to reach the FS_IOC_FIEMAP implementation.
>
> I assume you mean FIGETBSZ? The FS_IOC_FIEMAP IOCTL is the one that returns
> file extent maps, so that user space can reason about whether a file is stored
> in a consecutive way on disk.
I meant FS_IOC_FIEMAP for regular files.
>
>
> > I'd like to avoid exceptions as in the current implementation of
> > get_required_ioctl_dev_access() with a switch/case either returning 0 or
> > LANDLOCK_ACCESS_FS_IOCTL_DEV (excluding the default case of course). An
> > alternative approach would be to group IOCTL command cases according to
> > their returned value, but I find it a bit more complex for no meaningful
> > gain. What do you think?
>
> I don't have strong opinions about it, as long as we don't accidentally mess up
> the fallbacks if this changes.
>
>
> > > In your implementation at [3], you were permitting FICLONE* and FIDEDUPERANGE,
> > > but not FS_IOC_ZERO_RANGE, which is like fallocate(). How are these cases
> > > different to each other? Is that on purpose?
> >
> > FICLONE* and FIDEDUPERANGE match device files and the
> > vfs_clone_file_range()/generic_file_rw_checks() check returns EINVAL for
> > device files. So there is no need to add exceptions for these commands.
> >
> > FS_IOC_ZERO_RANGE is only implemented for regular files (see
> > file_ioctl() call), so it is passed to device files.
>
> Makes sense :)
>
>
> —Günther
>
next prev parent reply other threads:[~2024-04-05 18:01 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-27 13:10 [PATCH v13 00/10] Landlock: IOCTL support Günther Noack
2024-03-27 13:10 ` [PATCH v13 01/10] landlock: Add IOCTL access right for character and block devices Günther Noack
2024-03-27 16:57 ` Mickaël Salaün
2024-03-28 12:01 ` Mickaël Salaün
2024-04-02 18:28 ` Günther Noack
2024-04-03 11:15 ` Mickaël Salaün
2024-04-05 16:17 ` Günther Noack
2024-04-05 16:22 ` Günther Noack
2024-04-05 18:04 ` Mickaël Salaün
2024-04-05 18:17 ` Kent Overstreet
2024-04-05 21:44 ` Günther Noack
2024-04-05 18:01 ` Mickaël Salaün [this message]
2024-03-27 13:10 ` [PATCH v13 02/10] selftests/landlock: Test IOCTL support Günther Noack
2024-03-27 16:58 ` Mickaël Salaün
2024-03-27 13:10 ` [PATCH v13 03/10] selftests/landlock: Test IOCTL with memfds Günther Noack
2024-03-27 13:10 ` [PATCH v13 04/10] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH) Günther Noack
2024-03-27 13:10 ` [PATCH v13 05/10] selftests/landlock: Test IOCTLs on named pipes Günther Noack
2024-03-27 13:10 ` [PATCH v13 06/10] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets Günther Noack
2024-03-27 13:10 ` [PATCH v13 07/10] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV Günther Noack
2024-03-27 13:10 ` [PATCH v13 08/10] landlock: Document IOCTL support Günther Noack
2024-03-27 13:10 ` [PATCH v13 09/10] MAINTAINERS: Notify Landlock maintainers about changes to fs/ioctl.c Günther Noack
2024-03-27 13:10 ` [PATCH v13 10/10] fs/ioctl: Add a comment to keep the logic in sync with the Landlock LSM Günther Noack
2024-03-28 12:11 ` Mickaël Salaün
2024-03-28 13:08 ` Paul Moore
2024-03-28 16:43 ` Mickaël Salaün
2024-03-28 17:06 ` Paul Moore
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=20240405.oPhaosiel1ni@digikod.net \
--to=mic@digikod.net \
--cc=allenwebb@google.com \
--cc=arnd@arndb.de \
--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=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 NNTP newsgroup(s).