From: "Günther Noack" <gnoack@google.com>
To: Paul Moore <paul@paul-moore.com>
Cc: "Mickaël Salaün" <mic@digikod.net>,
"Arnd Bergmann" <arnd@arndb.de>,
"Dave Chinner" <david@fromorbit.com>,
"Christian Brauner" <brauner@kernel.org>,
"Allen Webb" <allenwebb@google.com>,
"Dmitry Torokhov" <dtor@google.com>,
"Jeff Xu" <jeffxu@google.com>,
"Jorge Lucangeli Obes" <jorgelo@chromium.org>,
"Konstantin Meskhidze" <konstantin.meskhidze@huawei.com>,
"Matt Bobrowski" <repnop@google.com>,
linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org
Subject: Re: [RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers
Date: Sat, 9 Mar 2024 09:14:24 +0100 [thread overview]
Message-ID: <ZewaYKO073V7P6Qy@google.com> (raw)
In-Reply-To: <CAHC9VhRnUbu2jRwUhLGboAgus_oFEPyddu=mv-OMLg93HHk17w@mail.gmail.com>
On Fri, Mar 08, 2024 at 05:25:21PM -0500, Paul Moore wrote:
> On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün <mic@digikod.net> wrote:
> > On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote:
> > > On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > Let's replace "safe IOCTL" with "IOCTL always allowed in a Landlock
> > > > sandbox".
> > >
> > > Which is a problem from a LSM perspective as we want to avoid hooks
> > > which are tightly bound to a single LSM or security model. It's okay
> > > if a new hook only has a single LSM implementation, but the hook's
> > > definition should be such that it is reasonably generalized to support
> > > multiple LSM/models.
> >
> > As any new hook, there is a first user. Obviously this new hook would
> > not be restricted to Landlock, it is a generic approach. I'm pretty
> > sure a few hooks are only used by one LSM though. ;)
>
> Sure, as I said above, it's okay for there to only be a single LSM
> implementation, but the basic idea behind the hook needs to have some
> hope of being generic. Your "let's redefine a safe ioctl as 'IOCTL
> always allowed in a Landlock sandbox'" doesn't fill me with confidence
> about the hook being generic; who knows, maybe it will be, but in the
> absence of a patch, I'm left with descriptions like those.
FWIW, the existing IOCTL hook is used in the following places:
* TOMOYO: seemingly configurable per IOCTL command? (I did not dig deeper)
* SELinux: has a hardcoded switch of IOCTL commands, some with special checks.
These are also a subset of the do_vfs_ioctl() commands,
plus KDSKBENT, KDSKBSENT (from ioctl_console(2)).
* Smack: Decomposes the IOCTL command number to look at the _IOC_WRITE and
_IOC_READ bits. (This is a known problematic approach, because (1) these bits
describe whether the argument is getting read or written, not whether the
operation is a mutating one, and (2) some IOCTL commands do not adhere to the
convention and don't use these macros)
AppArmor does not use the LSM IOCTL hook.
> > > I understand that this makes things a bit more
> > > complicated for Landlock's initial ioctl implementation, but
> > > considering my thoughts above and the fact that Landlock's ioctl
> > > protections are still evolving I'd rather not add a lot of extra hooks
> > > right now.
> >
> > Without this hook, we'll need to rely on a list of allowed IOCTLs, which
> > will be out-of-sync eventually. It would be a maintenance burden and an
> > hacky approach.
>
> Welcome to the painful world of a LSM developer, ioctls are not the
> only place where this is a problem, and it should be easy enough to
> watch for changes in the ioctl list and update your favorite LSM
> accordingly. Honestly, I think that is kinda the right thing anyway,
> I'm skeptical that one could have a generic solution that would
> automatically allow or disallow a new ioctl without potentially
> breaking your favorite LSM's security model. If a new ioctl is
> introduced it seems like having someone manually review it's impact on
> your LSM would be a good idea.
We are concerned that we will miss a change in do_vfs_ioctl(), which we would
like to reflect in the matching Landlock code. Do other LSMs have any
approaches for that which go beyond just watching the do_vfs_ioctl()
implementation for changes?
> > We're definitely open to new proposals, but until now this is the best
> > approach we found from a maintenance, performance, and security point of
> > view.
>
> At this point it's probably a good idea to post another RFC patch with
> your revised idea, if nothing else it will help rule out any
> confusion. While I remain skeptical, perhaps I am misunderstanding
> the design and you'll get my apology and an ACK, but be warned that as
> of right now I'm not convinced.
Thanks you for your feedback!
Here is V10 with the approach where we use a new LSM hook:
https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/
I hope this helps to clarify the approach a bit. I'm explaining it in more
detail again in the commit which adds the LSM hook, including a call graph, and
avoiding the word "safe" this time ;-)
Let me know what you think!
Thanks!
—Günther
next prev parent reply other threads:[~2024-03-09 8:14 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
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 [this message]
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=ZewaYKO073V7P6Qy@google.com \
--to=gnoack@google.com \
--cc=allenwebb@google.com \
--cc=arnd@arndb.de \
--cc=brauner@kernel.org \
--cc=david@fromorbit.com \
--cc=dtor@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).