From: "Günther Noack" <gnoack@google.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: "Mickaël Salaün" <mic@digikod.net>,
linux-security-module@vger.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,
"Christian Brauner" <brauner@kernel.org>
Subject: Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
Date: Tue, 26 Mar 2024 14:09:47 +0100 [thread overview]
Message-ID: <ZgLJG0aN0psur5Z7@google.com> (raw)
In-Reply-To: <b6a2a782-894a-461c-8fc1-9a3669545633@app.fastmail.com>
On Tue, Mar 26, 2024 at 12:58:42PM +0100, Arnd Bergmann wrote:
> On Tue, Mar 26, 2024, at 11:10, Mickaël Salaün wrote:
> > On Tue, Mar 26, 2024 at 10:33:23AM +0100, Arnd Bergmann wrote:
> >> On Tue, Mar 26, 2024, at 09:32, Mickaël Salaün wrote:
> >> >
> >> > This is indeed a simpler solution but unfortunately this doesn't fit
> >> > well with the requirements for an access control, especially when we
> >> > need to log denied accesses. Indeed, with this approach, the LSM (or
> >> > any other security mechanism) that returns ENOFILEOPS cannot know for
> >> > sure if the related request will allowed or not, and then it cannot
> >> > create reliable logs (unlike with EACCES or EPERM).
> >>
> >> Where does the requirement come from specifically, i.e.
> >> who is the consumer of that log?
> >
> > The audit framework may be used by LSMs to log denials.
> >
> >>
> >> Even if the log doesn't tell you directly whether the ioctl
> >> was ultimately denied, I would think logging the ENOFILEOPS
> >> along with the command number is enough to reconstruct what
> >> actually happened from reading the log later.
> >
> > We could indeed log ENOFILEOPS but that could include a lot of allowed
> > requests and we usually only want unlegitimate access requests to be
> > logged. Recording all ENOFILEOPS would mean 1/ that logs would be
> > flooded by legitimate requests and 2/ that user space log parsers would
> > need to deduce if a request was allowed or not, which require to know
> > the list of IOCTL commands implemented by fs/ioctl.c, which would defeat
> > the goal of this specific patch.
>
> Right, makes sense. Unfortunately that means I don't see any
> option that I think is actually better than what we have today,
> but that forces the use of a custom whitelist or extra logic in
> landlock.
>
> I didn't really mind having an extra hook for the callbacks
> in addition to the top-level one, but that was already nacked.
Thank you both for the review!
I agree, this approach would break logging.
As you both also said, I also think this leads us back to the approach
where we hardcode the allow-list of permitted IOCTL commands in the
file_ioctl hook.
I think this approach has the following upsides:
1. Landlock's (future) audit logging will be able to log exactly
which IOCTL commands were denied.
2. The allow-list of permitted IOCTL commands can be reasoned about
locally and does not accidentally change as a side-effect of a
change to the implementation of fs/ioctl.c.
A risk that we have is:
3. We might miss changes to fs/ioctl.c which we might want to
reflect in Landlock.
To think about 2 and 3 in more concrete terms, I categorized the
scenarios in which IOCTL cmd implementations can get added to or
removed from the do_vfs_ioctl() layer:
Case A: New cmd added to do_vfs_ioctl layer
We want to double check whether this cmd should be included in
Landlock's allow list. (Because the command is new, there are no
existing users of the IOCTL command, so we can't break anyone and
we it probably does not require to be made explicit in Landlock's
ABI versioning scheme.)
==> We need to catch these changes early,
and should do Landlock-side changes in sync.
Case B: Existing cmd removed from do_vfs_ioctl layer
(This case is unlikely, as it would be a backwards-incompatible
change in the ioctl interface.)
Case C: Existing cmd is moved from f_ops->..._ioctl() to do_vfs_ioctl()
(For regular files, I think this has happened for the XFS
"reflink" ioctls before, which became features in other COW file
systems as well.)
If such a change happens for device files (which are in scope for
Landlock's IOCTL feature), we should catch these changes. We
should consider whether the affected IOCTL command should be
allow-listed. Strictly speaking, if we allow-list the cmd, which
was previously not allow-listed, this would mean that Landlock's
DEV_IOCTL feature would have different semantics than before
(permitting an additional cmd), and it would potentially be a
backwards-incompatible change that need to be made explicit in
Landlock's ABI versioning.
Case D: Existing cmd is moved from do_vfs_ioctl() to f_ops->..._ioctl()
(This case seems also very unlikely to me because it decentralizes
the reasoning about these IOCTL APIs which are currently centrally
controlled and must stay backwards compatible.)
So -- a proposal:
* Keep the original implementation of fs/ioctl.c
* Implement Landlock's file_ioctl hook with a switch(cmd) where we list
the permitted IOCTL commands from do_vfs_ioctl.
* Make sure Landlock maintainers learn about changes to do_vfs_ioctl():
* Put a warning on top of do_vfs_ioctl() to loop in Landlock
maintainers
* Set up automation to catch such changes?
Open questions are:
* Mickaël, do you think we should use a more device-file-specific
subset of IOCTL commands again, or would you prefer to stick to the
full list of all IOCTL commands implemented in do_vfs_ioctl()?
* Regarding automation:
We could additionally set up some automation to watch changes to
do_vfs_ioctl(). Given the low rate of change in that corner, we
might get away with extracting the relevant portion of the C file
(awk '/^static int do_vfs_ioctl/, /^\}/' fs/ioctl.c) and watching
for any changes. It is brittle, but the rate of changes is low, and
reasoning about source code is difficult and error prone as well.
In an ideal world this could maybe be part of the kernel test
suites, but I still have not found the right place where this could
be hooked in. Do you have any thoughts on this?
Thanks,
—Günther
next prev parent reply other threads:[~2024-03-26 13:09 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
2024-03-25 13:39 ` [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks Günther Noack
2024-03-25 14:28 ` Günther Noack
2024-03-25 15:19 ` Arnd Bergmann
2024-03-26 8:32 ` Mickaël Salaün
2024-03-26 9:33 ` Arnd Bergmann
2024-03-26 10:10 ` Mickaël Salaün
2024-03-26 11:58 ` Arnd Bergmann
2024-03-26 13:09 ` Günther Noack [this message]
2024-03-26 14:28 ` Mickaël Salaün
2024-03-26 18:52 ` Paul Moore
2024-03-25 13:39 ` [PATCH v12 2/9] landlock: Add IOCTL access right for character and block devices Günther Noack
2024-03-25 13:39 ` [PATCH v12 3/9] selftests/landlock: Test IOCTL support Günther Noack
2024-03-25 13:39 ` [PATCH v12 4/9] selftests/landlock: Test IOCTL with memfds Günther Noack
2024-03-25 13:40 ` [PATCH v12 5/9] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH) Günther Noack
2024-03-25 13:40 ` [PATCH v12 6/9] selftests/landlock: Test IOCTLs on named pipes Günther Noack
2024-03-25 13:40 ` [PATCH v12 7/9] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets Günther Noack
2024-03-25 13:40 ` [PATCH v12 8/9] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV Günther Noack
2024-03-25 13:40 ` [PATCH v12 9/9] 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=ZgLJG0aN0psur5Z7@google.com \
--to=gnoack@google.com \
--cc=allenwebb@google.com \
--cc=arnd@arndb.de \
--cc=brauner@kernel.org \
--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).