From: Dave Chinner <david@fromorbit.com>
To: Paul Moore <paul@paul-moore.com>
Cc: "Günther Noack" <gnoack@google.com>,
"Mickaël Salaün" <mic@digikod.net>,
"Arnd Bergmann" <arnd@arndb.de>,
"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: Fri, 8 Mar 2024 10:09:02 +1100 [thread overview]
Message-ID: <ZepJDgvxVkhZ5xYq@dread.disaster.area> (raw)
In-Reply-To: <CAHC9VhT1thow+4fo0qbJoempGu8+nb6_26s16kvVSVVAOWdtsQ@mail.gmail.com>
On Thu, Mar 07, 2024 at 03:40:44PM -0500, Paul Moore wrote:
> On Thu, Mar 7, 2024 at 7:57 AM Günther Noack <gnoack@google.com> wrote:
> > On Thu, Mar 07, 2024 at 01:21:48PM +0100, Arnd Bergmann wrote:
> > > On Thu, Mar 7, 2024, at 13:15, Christian Brauner wrote:
> > > > On Wed, Mar 06, 2024 at 04:18:53PM +0100, Arnd Bergmann wrote:
> > > >> On Wed, Mar 6, 2024, at 14:47, Mickaël Salaün wrote:
> > > >> >
> > > >> > Arnd, Christian, Paul, are you OK with this new hook proposal?
> > > >>
> > > >> I think this sounds better. It would fit more closely into
> > > >> the overall structure of the ioctl handlers with their multiple
> > > >> levels, where below vfs_ioctl() calling into f_ops->unlocked_ioctl,
> > > >> you have the same structure for sockets and blockdev, and
> > > >> then additional levels below that and some weirdness for
> > > >> things like tty, scsi or cdrom.
> > > >
> > > > So an additional security hook called from tty, scsi, or cdrom?
> > > > And the original hook is left where it is right now?
> > >
> > > For the moment, I think adding another hook in vfs_ioctl()
> > > and the corresponding compat path would do what Mickaël
> > > wants. Beyond that, we could consider having hooks in
> > > socket and block ioctls if needed as they are easy to
> > > filter out based on inode->i_mode.
> > >
> > > The tty/scsi/cdrom hooks would be harder to do, let's assume
> > > for now that we don't need them.
> >
> > Thank you all for the help!
> >
> > Yes, tty/scsi/cdrom are just examples. We do not need special features for
> > these for Landlock right now.
> >
> > What I would do is to invoke the new LSM hook in the following two places in
> > fs/ioctl.c:
> >
> > 1) at the top of vfs_ioctl()
> > 2) at the top of ioctl_compat()
> >
> > (Both of these functions are just invoking the f_op->unlocked_ioctl() and
> > f_op->compat_ioctl() operations with a safeguard for that being a NULL pointer.)
> >
> > The intent is that the new hook gets called everytime before an ioctl is sent to
> > these IOCTL operations in f_op, so that the LSM can distinguish cleanly between
> > the "safe" IOCTLs that are implemented fully within fs/ioctl.c and the
> > "potentially unsafe" IOCTLs which are implemented by these hooks (as it is
> > unrealistic for us to holistically reason about the safety of all possible
> > implementations).
> >
> > The alternative approach where we try to do the same based on the existing LSM
> > IOCTL hook resulted in the patch further up in this mail thread - it involves
> > maintaining a list of "safe" IOCTL commands, and it is difficult to guarantee
> > that these lists of IOCTL commands stay in sync.
>
> I need some more convincing as to why we need to introduce these new
> hooks, or even the vfs_masked_device_ioctl() classifier as originally
> proposed at the top of this thread. I believe I understand why
> Landlock wants this, but I worry that we all might have different
> definitions of a "safe" ioctl list, and encoding a definition into the
> LSM hooks seems like a bad idea to me.
I have no idea what a "safe" ioctl means here. Subsystems already
restrict ioctls that can do damage if misused to CAP_SYS_ADMIN, so
"safe" clearly means something different here.
> At this point in time, I think I'd rather see LSMs that care about
> ioctls maintaining their own list of "safe" ioctls and after a while
> if it looks like everyone is in agreement (VFS folks, individual LSMs,
> etc.) we can look into either an ioctl classifier or multiple LSM
> ioctl hooks focused on different categories of ioctls.
From the perspective of a VFS and subsystem developer, I really have
no clue what would make a "safe" ioctl from a LSM perspective, and I
very much doubt an LSM developer has any clue whether deep, dark
subsystem ioctls are "safe" to allow, or even what would stop
working if they decided something was not "safe".
This just seems like a complex recipe for creating unusable and/or
impossible to configure/secure systems to me.
-Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2024-03-07 23:09 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 [this message]
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=ZepJDgvxVkhZ5xYq@dread.disaster.area \
--to=david@fromorbit.com \
--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=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).