* [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
@ 2024-03-25 13:39 ` Günther Noack
2024-03-25 14:28 ` Günther Noack
` (2 more replies)
2024-03-25 13:39 ` [PATCH v12 2/9] landlock: Add IOCTL access right for character and block devices Günther Noack
` (7 subsequent siblings)
8 siblings, 3 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:39 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack, Christian Brauner
If security_file_ioctl or security_file_ioctl_compat return
ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
command, but only as long as the IOCTL command is implemented directly
in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
f_ops->compat_ioctl operations, which are defined by the given file.
The possible return values for security_file_ioctl and
security_file_ioctl_compat are now:
* 0 - to permit the IOCTL
* ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
back to the file implementation.
* any other error - to forbid the IOCTL and return that error
This is an alternative to the previously discussed approaches [1] and [2],
and implements the proposal from [3].
Cc: Christian Brauner <brauner@kernel.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Mickaël Salaün <mic@digikod.net>
Cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/20240309075320.160128-2-gnoack@google.com [1]
Link: https://lore.kernel.org/r/20240322151002.3653639-2-gnoack@google.com/ [2]
Link: https://lore.kernel.org/r/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@app.fastmail.com/ [3]
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Günther Noack <gnoack@google.com>
---
fs/ioctl.c | 25 ++++++++++++++++++++-----
include/linux/security.h | 6 ++++++
security/security.c | 10 ++++++++--
3 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..8244354ad04d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -828,7 +828,7 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
case FIONREAD:
if (!S_ISREG(inode->i_mode))
- return vfs_ioctl(filp, cmd, arg);
+ return -ENOIOCTLCMD;
return put_user(i_size_read(inode) - filp->f_pos,
(int __user *)argp);
@@ -858,17 +858,24 @@ SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, unsigned long, arg)
{
struct fd f = fdget(fd);
int error;
+ bool use_file_ops = true;
if (!f.file)
return -EBADF;
error = security_file_ioctl(f.file, cmd, arg);
- if (error)
+ if (error == -ENOFILEOPS)
+ use_file_ops = false;
+ else if (error)
goto out;
error = do_vfs_ioctl(f.file, fd, cmd, arg);
- if (error == -ENOIOCTLCMD)
- error = vfs_ioctl(f.file, cmd, arg);
+ if (error == -ENOIOCTLCMD) {
+ if (use_file_ops)
+ error = vfs_ioctl(f.file, cmd, arg);
+ else
+ error = -EACCES;
+ }
out:
fdput(f);
@@ -916,12 +923,15 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
{
struct fd f = fdget(fd);
int error;
+ bool use_file_ops = true;
if (!f.file)
return -EBADF;
error = security_file_ioctl_compat(f.file, cmd, arg);
- if (error)
+ if (error == -ENOFILEOPS)
+ use_file_ops = false;
+ else if (error)
goto out;
switch (cmd) {
@@ -967,6 +977,11 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
if (error != -ENOIOCTLCMD)
break;
+ if (!use_file_ops) {
+ error = -EACCES;
+ break;
+ }
+
if (f.file->f_op->compat_ioctl)
error = f.file->f_op->compat_ioctl(f.file, cmd, arg);
if (error == -ENOIOCTLCMD)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..b769dc888d07 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -248,6 +248,12 @@ static const char * const kernel_load_data_str[] = {
__kernel_read_file_id(__data_id_stringify)
};
+/*
+ * Returned by security_file_ioctl and security_file_ioctl_compat to indicate
+ * that the IOCTL request may not be dispatched to the file's f_ops IOCTL impl.
+ */
+#define ENOFILEOPS 532
+
static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
{
if ((unsigned)id >= LOADING_MAX_ID)
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..000c54a1e541 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2719,7 +2719,10 @@ void security_file_free(struct file *file)
* value. When @arg represents a user space pointer, it should never be used
* by the security module.
*
- * Return: Returns 0 if permission is granted.
+ * Return: Returns 0 if permission is granted. Returns -ENOFILEOPS if
+ * permission is granted for IOCTL commands that do not get handled by
+ * f_ops->unlocked_ioctl(). Returns another negative error code is
+ * permission is denied.
*/
int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
@@ -2736,7 +2739,10 @@ EXPORT_SYMBOL_GPL(security_file_ioctl);
* Compat version of security_file_ioctl() that correctly handles 32-bit
* processes running on 64-bit kernels.
*
- * Return: Returns 0 if permission is granted.
+ * Return: Returns 0 if permission is granted. Returns -ENOFILEOPS if permission
+ * is granted for IOCTL commands that do not get handled by
+ * f_ops->compat_ioctl(). Returns another negative error code is
+ * permission is denied.
*/
int security_file_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
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 18:52 ` Paul Moore
2 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 14:28 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Christian Brauner
On Mon, Mar 25, 2024 at 01:39:56PM +0000, Günther Noack wrote:
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d0eb20f90b26..b769dc888d07 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -248,6 +248,12 @@ static const char * const kernel_load_data_str[] = {
> __kernel_read_file_id(__data_id_stringify)
> };
>
> +/*
> + * Returned by security_file_ioctl and security_file_ioctl_compat to indicate
> + * that the IOCTL request may not be dispatched to the file's f_ops IOCTL impl.
> + */
> +#define ENOFILEOPS 532
FYI, the thinking here was:
* I could not find an existing error code that seemed to have a similar meaning,
which we could reuse.
* At the same time, the meaning of this error code is so special that the approach
of adding it to kernel-private codes in include/linux/errno.h also seemed wrong.
* The number 532 is just one higher than the highest code in include/linux/errno.h
Suggestions welcome :)
—Günther
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
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 18:52 ` Paul Moore
2 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2024-03-25 15:19 UTC (permalink / raw)
To: Günther Noack, linux-security-module,
Mickaël Salaün
Cc: Jeff Xu, Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov,
Paul Moore, Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
On Mon, Mar 25, 2024, at 14:39, Günther Noack wrote:
> If security_file_ioctl or security_file_ioctl_compat return
> ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
> command, but only as long as the IOCTL command is implemented directly
> in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
> f_ops->compat_ioctl operations, which are defined by the given file.
>
> The possible return values for security_file_ioctl and
> security_file_ioctl_compat are now:
>
> * 0 - to permit the IOCTL
> * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
> back to the file implementation.
> * any other error - to forbid the IOCTL and return that error
>
> This is an alternative to the previously discussed approaches [1] and [2],
> and implements the proposal from [3].
Thanks for trying it out, I think this is a good solution
and I like how the code turned out.
One small thing that I believe needs some extra changes:
> @@ -967,6 +977,11 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd,
> unsigned int, cmd,
> if (error != -ENOIOCTLCMD)
> break;
>
> + if (!use_file_ops) {
> + error = -EACCES;
> + break;
> + }
> +
> if (f.file->f_op->compat_ioctl)
> error = f.file->f_op->compat_ioctl(f.file, cmd, arg);
> if (error == -ENOIOCTLCMD)
The compat FIONREAD handler now ends up calling ->compat_ioctl()
where it used to call ->ioctl(). I think this means we need to
audit every driver that implements its own
FIONREAD/SIOCINQ/TIOCINQ to make sure it has a working compat
implementation.
I have done one pass through all such drivers and think the
change below should be sufficient for all of them, but please
have another look. Feel free to fold this change into your
patch. The pipe.c change also fixes an existing bug with
IOC_WATCH_QUEUE_SET_SIZE/IOC_WATCH_QUEUE_SET_FILTER, so that
may need to be a separate patch and get backported.
Arnd
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c1..2e5b495a5606 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2891,6 +2891,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
int retval = -ENOIOCTLCMD;
switch (cmd) {
+ case TIOCINQ:
case TIOCOUTQ:
case TIOCSTI:
case TIOCGWINSZ:
diff --git a/fs/pipe.c b/fs/pipe.c
index 50c8a8596b52..a6ebb351ea4b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -652,6 +652,14 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
}
}
+static long pipe_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ if (cmd == IOC_WATCH_QUEUE_SET_SIZE)
+ return pipe_ioctl(filp, cmd, arg);
+
+ return compat_ptr_ioctl(filp, cmd, arg);
+}
+
/* No kernel lock held - fine */
static __poll_t
pipe_poll(struct file *filp, poll_table *wait)
@@ -1234,6 +1242,7 @@ const struct file_operations pipefifo_fops = {
.write_iter = pipe_write,
.poll = pipe_poll,
.unlocked_ioctl = pipe_ioctl,
+ .compat_ioctl = pipe_compat_ioctl,
.release = pipe_release,
.fasync = pipe_fasync,
.splice_write = iter_file_splice_write,
diff --git a/net/socket.c b/net/socket.c
index e5f3af49a8b6..bb4fa51fe4ca 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3496,6 +3496,7 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
case SIOCSARP:
case SIOCGARP:
case SIOCDARP:
+ case SIOCINQ:
case SIOCOUTQ:
case SIOCOUTQNSD:
case SIOCATMARK:
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-25 15:19 ` Arnd Bergmann
@ 2024-03-26 8:32 ` Mickaël Salaün
2024-03-26 9:33 ` Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Mickaël Salaün @ 2024-03-26 8:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Günther Noack, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
On Mon, Mar 25, 2024 at 04:19:25PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 25, 2024, at 14:39, Günther Noack wrote:
> > If security_file_ioctl or security_file_ioctl_compat return
> > ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
> > command, but only as long as the IOCTL command is implemented directly
> > in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
> > f_ops->compat_ioctl operations, which are defined by the given file.
> >
> > The possible return values for security_file_ioctl and
> > security_file_ioctl_compat are now:
> >
> > * 0 - to permit the IOCTL
> > * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
> > back to the file implementation.
> > * any other error - to forbid the IOCTL and return that error
> >
> > This is an alternative to the previously discussed approaches [1] and [2],
> > and implements the proposal from [3].
>
> Thanks for trying it out, I think this is a good solution
> and I like how the code turned out.
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).
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
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
0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2024-03-26 9:33 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
On Tue, Mar 26, 2024, at 09:32, Mickaël Salaün wrote:
> On Mon, Mar 25, 2024 at 04:19:25PM +0100, Arnd Bergmann wrote:
>> On Mon, Mar 25, 2024, at 14:39, Günther Noack wrote:
>> > If security_file_ioctl or security_file_ioctl_compat return
>> > ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
>> > command, but only as long as the IOCTL command is implemented directly
>> > in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
>> > f_ops->compat_ioctl operations, which are defined by the given file.
>> >
>> > The possible return values for security_file_ioctl and
>> > security_file_ioctl_compat are now:
>> >
>> > * 0 - to permit the IOCTL
>> > * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
>> > back to the file implementation.
>> > * any other error - to forbid the IOCTL and return that error
>> >
>> > This is an alternative to the previously discussed approaches [1] and [2],
>> > and implements the proposal from [3].
>>
>> Thanks for trying it out, I think this is a good solution
>> and I like how the code turned out.
>
> 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?
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.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-26 9:33 ` Arnd Bergmann
@ 2024-03-26 10:10 ` Mickaël Salaün
2024-03-26 11:58 ` Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Mickaël Salaün @ 2024-03-26 10:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Günther Noack, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
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:
> > On Mon, Mar 25, 2024 at 04:19:25PM +0100, Arnd Bergmann wrote:
> >> On Mon, Mar 25, 2024, at 14:39, Günther Noack wrote:
> >> > If security_file_ioctl or security_file_ioctl_compat return
> >> > ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
> >> > command, but only as long as the IOCTL command is implemented directly
> >> > in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
> >> > f_ops->compat_ioctl operations, which are defined by the given file.
> >> >
> >> > The possible return values for security_file_ioctl and
> >> > security_file_ioctl_compat are now:
> >> >
> >> > * 0 - to permit the IOCTL
> >> > * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
> >> > back to the file implementation.
> >> > * any other error - to forbid the IOCTL and return that error
> >> >
> >> > This is an alternative to the previously discussed approaches [1] and [2],
> >> > and implements the proposal from [3].
> >>
> >> Thanks for trying it out, I think this is a good solution
> >> and I like how the code turned out.
> >
> > 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-26 10:10 ` Mickaël Salaün
@ 2024-03-26 11:58 ` Arnd Bergmann
2024-03-26 13:09 ` Günther Noack
0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2024-03-26 11:58 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
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.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-26 11:58 ` Arnd Bergmann
@ 2024-03-26 13:09 ` Günther Noack
2024-03-26 14:28 ` Mickaël Salaün
0 siblings, 1 reply; 19+ messages in thread
From: Günther Noack @ 2024-03-26 13:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mickaël Salaün, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
2024-03-26 13:09 ` Günther Noack
@ 2024-03-26 14:28 ` Mickaël Salaün
0 siblings, 0 replies; 19+ messages in thread
From: Mickaël Salaün @ 2024-03-26 14:28 UTC (permalink / raw)
To: Günther Noack
Cc: Arnd Bergmann, linux-security-module, Jeff Xu,
Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov, Paul Moore,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
On Tue, Mar 26, 2024 at 02:09:47PM +0100, Günther Noack wrote:
> 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.)
>
Excellent summary! You should put a link to this email in the commit
message and quickly explain why we ended up here.
>
>
> 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
Yes please.
> * 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()?
We should stick to the device-file-specific IOCTL commands [1] but we
should probably complete the switch-case with commented cases (in the
same order as in do_vfs_ioctl) to know which one were reviewed (as in
[1]). These helpers should now be static and in security/landlock/fs.c
[1] https://lore.kernel.org/r/20240219183539.2926165-1-mic@digikod.net
BTW, there are now two new IOCTL commands (FS_IOC_GETUUID and
FS_IOC_GETFSSYSFSPATH) masking device-specific IOCTL ones.
>
> * 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?
I think this change should be enough for now:
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12222,6 +12222,7 @@ W: https://landlock.io
T: git https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
F: Documentation/security/landlock.rst
F: Documentation/userspace-api/landlock.rst
+F: fs/ioctl.c
F: include/uapi/linux/landlock.h
F: samples/landlock/
F: security/landlock/
It should be OK considered the number of patches matching this file: a
subset of https://lore.kernel.org/all/?q=dfn%3Afs%2Fioctl.c
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
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 18:52 ` Paul Moore
2 siblings, 0 replies; 19+ messages in thread
From: Paul Moore @ 2024-03-26 18:52 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Mickaël Salaün, Jeff Xu,
Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb, Dmitry Torokhov,
Konstantin Meskhidze, Matt Bobrowski, linux-fsdevel,
Christian Brauner
On Mon, Mar 25, 2024 at 9:40 AM Günther Noack <gnoack@google.com> wrote:
>
> If security_file_ioctl or security_file_ioctl_compat return
> ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
> command, but only as long as the IOCTL command is implemented directly
> in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
> f_ops->compat_ioctl operations, which are defined by the given file.
>
> The possible return values for security_file_ioctl and
> security_file_ioctl_compat are now:
>
> * 0 - to permit the IOCTL
> * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
> back to the file implementation.
> * any other error - to forbid the IOCTL and return that error
At this point I think this thread has resolved itself, but I wanted to
add a quick comment for those who may stumble across this in the
future ... I want to discourage magic return values in the LSM hooks
as much as possible; they have caused issues in the past and I suspect
they will continue to do so in the future (although now that we have
proper function header comments the risk may be slightly lower). If
there is absolutely no way around it, then that's okay, but if
possible I would prefer we stick with the 0:allowed, !0:rejected model
for the LSM hook return values.
> This is an alternative to the previously discussed approaches [1] and [2],
> and implements the proposal from [3].
>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Mickaël Salaün <mic@digikod.net>
> Cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/20240309075320.160128-2-gnoack@google.com [1]
> Link: https://lore.kernel.org/r/20240322151002.3653639-2-gnoack@google.com/ [2]
> Link: https://lore.kernel.org/r/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@app.fastmail.com/ [3]
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
> fs/ioctl.c | 25 ++++++++++++++++++++-----
> include/linux/security.h | 6 ++++++
> security/security.c | 10 ++++++++--
> 3 files changed, 34 insertions(+), 7 deletions(-)
--
paul-moore.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v12 2/9] landlock: Add IOCTL access right for character and block devices
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 13:39 ` Günther Noack
2024-03-25 13:39 ` [PATCH v12 3/9] selftests/landlock: Test IOCTL support Günther Noack
` (6 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:39 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack, Christian Brauner
Introduces the LANDLOCK_ACCESS_FS_IOCTL_DEV right
and increments the Landlock ABI version to 5.
This access right applies to device-custom IOCTL commands
when they are invoked on block or character device files.
Like the truncate right, this right is associated with a file
descriptor at the time of open(2), and gets respected even when the
file descriptor is used outside of the thread which it was originally
opened in.
Therefore, a newly enabled Landlock policy does not apply to file
descriptors which are already open.
If the LANDLOCK_ACCESS_FS_IOCTL_DEV right is handled, only a small
number of safe IOCTL commands will be permitted on newly opened device
files. These include FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC, as well
as other IOCTL commands for regular files which are implemented in
fs/ioctl.c.
Noteworthy scenarios which require special attention:
TTY devices are often passed into a process from the parent process,
and so a newly enabled Landlock policy does not retroactively apply to
them automatically. In the past, TTY devices have often supported
IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
letting callers control the TTY input buffer (and simulate
keypresses). This should be restricted to CAP_SYS_ADMIN programs on
modern kernels though.
Known limitations:
The LANDLOCK_ACCESS_FS_IOCTL_DEV access right is a coarse-grained
control over IOCTL commands.
Landlock users may use path-based restrictions in combination with
their knowledge about the file system layout to control what IOCTLs
can be done.
Cc: Paul Moore <paul@paul-moore.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Günther Noack <gnoack@google.com>
---
include/uapi/linux/landlock.h | 35 +++++++++++----
security/landlock/fs.c | 45 ++++++++++++++++++--
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 8 +++-
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/fs_test.c | 5 ++-
6 files changed, 80 insertions(+), 17 deletions(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677539..193733d833b1 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -128,7 +128,7 @@ struct landlock_net_port_attr {
* files and directories. Files or directories opened before the sandboxing
* are not subject to these restrictions.
*
- * A file can only receive these access rights:
+ * The following access rights apply only to files:
*
* - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
* - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
@@ -138,12 +138,13 @@ struct landlock_net_port_attr {
* - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
* - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
* :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
- * ``O_TRUNC``. Whether an opened file can be truncated with
- * :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
- * same way as read and write permissions are checked during
- * :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
- * %LANDLOCK_ACCESS_FS_WRITE_FILE. This access right is available since the
- * third version of the Landlock ABI.
+ * ``O_TRUNC``. This access right is available since the third version of the
+ * Landlock ABI.
+ *
+ * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used
+ * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as
+ * read and write permissions are checked during :manpage:`open(2)` using
+ * %LANDLOCK_ACCESS_FS_READ_FILE and %LANDLOCK_ACCESS_FS_WRITE_FILE.
*
* A directory can receive access rights related to files or directories. The
* following access right is applied to the directory itself, and the
@@ -198,13 +199,30 @@ struct landlock_net_port_attr {
* If multiple requirements are not met, the ``EACCES`` error code takes
* precedence over ``EXDEV``.
*
+ * The following access right applies both to files and directories:
+ *
+ * - %LANDLOCK_ACCESS_FS_IOCTL_DEV: Invoke :manpage:`ioctl(2)` commands on an opened
+ * character or block device.
+ *
+ * This access right applies to all `ioctl(2)` commands implemented by device
+ * drivers. However, the following common IOCTL commands continue to be
+ * invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL_DEV right:
+ *
+ * ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC``, ``FIOQSIZE``,
+ * ``FIFREEZE``, ``FITHAW``, ``FS_IOC_FIEMAP``, ``FIGETBSZ``, ``FICLONE``,
+ * ``FICLONERANGE``, ``FIDEDUPERANGE``, ``FS_IOC_GETFLAGS``,
+ * ``FS_IOC_SETFLAGS``, ``FS_IOC_FSGETXATTR``, ``FS_IOC_FSSETXATTR``
+ *
+ * This access right is available since the fifth version of the Landlock
+ * ABI.
+ *
* .. warning::
*
* It is currently not possible to restrict some file-related actions
* accessible through these syscall families: :manpage:`chdir(2)`,
* :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
* :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- * :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ * :manpage:`fcntl(2)`, :manpage:`access(2)`.
* Future Landlock evolutions will enable to restrict them.
*/
/* clang-format off */
@@ -223,6 +241,7 @@ struct landlock_net_port_attr {
#define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
#define LANDLOCK_ACCESS_FS_REFER (1ULL << 13)
#define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14)
+#define LANDLOCK_ACCESS_FS_IOCTL_DEV (1ULL << 15)
/* clang-format on */
/**
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c15559432d3d..650ae498e9a1 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -148,7 +148,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_IOCTL_DEV)
/* clang-format on */
/*
@@ -1335,8 +1336,10 @@ static int hook_file_alloc_security(struct file *const file)
static int hook_file_open(struct file *const file)
{
layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
- access_mask_t open_access_request, full_access_request, allowed_access;
- const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
+ access_mask_t open_access_request, full_access_request, allowed_access,
+ optional_access;
+ const struct inode *inode = file_inode(file);
+ const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
const struct landlock_ruleset *const dom =
get_fs_domain(landlock_cred(file->f_cred)->domain);
@@ -1354,6 +1357,10 @@ static int hook_file_open(struct file *const file)
* We look up more access than what we immediately need for open(), so
* that we can later authorize operations on opened files.
*/
+ optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
+ if (is_device)
+ optional_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
full_access_request = open_access_request | optional_access;
if (is_access_to_paths_allowed(
@@ -1410,6 +1417,36 @@ static int hook_file_truncate(struct file *const file)
return -EACCES;
}
+static int hook_file_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ const struct inode *inode = file_inode(file);
+ const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
+ access_mask_t required_access, allowed_access;
+
+ if (!is_device)
+ return 0;
+
+ /*
+ * It is the access rights at the time of opening the file which
+ * determine whether IOCTL can be used on the opened file later.
+ *
+ * The access right is attached to the opened file in hook_file_open().
+ */
+ required_access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
+ allowed_access = landlock_file(file)->allowed_access;
+ if ((allowed_access & required_access) == required_access)
+ return 0;
+
+ return -ENOFILEOPS;
+}
+
+static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return hook_file_ioctl(file, cmd, arg);
+}
+
static struct security_hook_list landlock_hooks[] __ro_after_init = {
LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
@@ -1432,6 +1469,8 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
LSM_HOOK_INIT(file_open, hook_file_open),
LSM_HOOK_INIT(file_truncate, hook_file_truncate),
+ LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
+ LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
};
__init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 93c9c6f91556..20fdb5ff3514 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
#define LANDLOCK_MAX_NUM_LAYERS 16
#define LANDLOCK_MAX_NUM_RULES U32_MAX
-#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_IOCTL_DEV
#define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
#define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS)
#define LANDLOCK_SHIFT_ACCESS_FS 0
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 6788e73b6681..9ae3dfa47443 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
.write = fop_dummy_write,
};
-#define LANDLOCK_ABI_VERSION 4
+#define LANDLOCK_ABI_VERSION 5
/**
* sys_landlock_create_ruleset - Create a new ruleset
@@ -321,7 +321,11 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
if (!path_beneath_attr.allowed_access)
return -ENOMSG;
- /* Checks that allowed_access matches the @ruleset constraints. */
+ /*
+ * Checks that allowed_access matches the @ruleset constraints and only
+ * consists of publicly visible access rights (as opposed to synthetic
+ * ones).
+ */
mask = landlock_get_raw_fs_access_mask(ruleset, 0);
if ((path_beneath_attr.allowed_access | mask) != mask)
return -EINVAL;
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 646f778dfb1e..d292b419ccba 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
};
- ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
+ ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION));
ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 2d6d9b43d958..0bcbbf594fd7 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -527,9 +527,10 @@ TEST_F_FORK(layout1, inval)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_IOCTL_DEV)
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
#define ACCESS_ALL ( \
ACCESS_FILE | \
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 3/9] selftests/landlock: Test IOCTL support
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 13:39 ` [PATCH v12 2/9] landlock: Add IOCTL access right for character and block devices Günther Noack
@ 2024-03-25 13:39 ` Günther Noack
2024-03-25 13:39 ` [PATCH v12 4/9] selftests/landlock: Test IOCTL with memfds Günther Noack
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:39 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
Exercises Landlock's IOCTL feature in different combinations of
handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, and in
different combinations of using files and directories.
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/fs_test.c | 236 ++++++++++++++++++++-
1 file changed, 233 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 0bcbbf594fd7..22229fe3e403 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -8,6 +8,7 @@
*/
#define _GNU_SOURCE
+#include <asm/termbits.h>
#include <fcntl.h>
#include <linux/landlock.h>
#include <linux/magic.h>
@@ -15,6 +16,7 @@
#include <stdio.h>
#include <string.h>
#include <sys/capability.h>
+#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/sendfile.h>
@@ -23,6 +25,12 @@
#include <sys/vfs.h>
#include <unistd.h>
+/*
+ * Intentionally included last to work around header conflict.
+ * See https://sourceware.org/glibc/wiki/Synchronizing_Headers.
+ */
+#include <linux/fs.h>
+
#include "common.h"
#ifndef renameat2
@@ -735,6 +743,9 @@ static int create_ruleset(struct __test_metadata *const _metadata,
}
for (i = 0; rules[i].path; i++) {
+ if (!rules[i].access)
+ continue;
+
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
rules[i].path);
}
@@ -3443,7 +3454,7 @@ TEST_F_FORK(layout1, truncate_unhandled)
LANDLOCK_ACCESS_FS_WRITE_FILE;
int ruleset_fd;
- /* Enable Landlock. */
+ /* Enables Landlock. */
ruleset_fd = create_ruleset(_metadata, handled, rules);
ASSERT_LE(0, ruleset_fd);
@@ -3526,7 +3537,7 @@ TEST_F_FORK(layout1, truncate)
LANDLOCK_ACCESS_FS_TRUNCATE;
int ruleset_fd;
- /* Enable Landlock. */
+ /* Enables Landlock. */
ruleset_fd = create_ruleset(_metadata, handled, rules);
ASSERT_LE(0, ruleset_fd);
@@ -3752,7 +3763,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
};
int fd, ruleset_fd;
- /* Enable Landlock. */
+ /* Enables Landlock. */
ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
ASSERT_LE(0, ruleset_fd);
enforce_ruleset(_metadata, ruleset_fd);
@@ -3829,6 +3840,16 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
ASSERT_EQ(0, close(socket_fds[1]));
}
+/* Invokes the FS_IOC_GETFLAGS IOCTL and returns its errno or 0. */
+static int test_fs_ioc_getflags_ioctl(int fd)
+{
+ uint32_t flags;
+
+ if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0)
+ return errno;
+ return 0;
+}
+
TEST(memfd_ftruncate)
{
int fd;
@@ -3845,6 +3866,215 @@ TEST(memfd_ftruncate)
ASSERT_EQ(0, close(fd));
}
+static int test_fionread_ioctl(int fd)
+{
+ size_t sz = 0;
+
+ if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
+ return errno;
+ return 0;
+}
+
+/* clang-format off */
+FIXTURE(ioctl) {};
+
+FIXTURE_SETUP(ioctl) {};
+
+FIXTURE_TEARDOWN(ioctl) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(ioctl)
+{
+ const __u64 handled;
+ const __u64 allowed;
+ const mode_t open_mode;
+ /*
+ * TCGETS is used as a characteristic device-specific IOCTL command.
+ * The logic is the same for other IOCTL commands as well.
+ */
+ const int expected_tcgets_result; /* terminal device IOCTL */
+ /*
+ * FIONREAD is implemented in fs/ioctl.c for regular files,
+ * but we do not blanket-permit it for devices.
+ */
+ const int expected_fionread_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_none) {
+ /* clang-format on */
+ .handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+ .allowed = 0,
+ .open_mode = O_RDWR,
+ .expected_tcgets_result = EACCES,
+ .expected_fionread_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_i) {
+ /* clang-format on */
+ .handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+ .allowed = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+ .open_mode = O_RDWR,
+ .expected_tcgets_result = 0,
+ .expected_fionread_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, unhandled) {
+ /* clang-format on */
+ .handled = LANDLOCK_ACCESS_FS_EXECUTE,
+ .allowed = LANDLOCK_ACCESS_FS_EXECUTE,
+ .open_mode = O_RDWR,
+ .expected_tcgets_result = 0,
+ .expected_fionread_result = 0,
+};
+
+static int test_fioqsize_ioctl(int fd)
+{
+ size_t sz;
+
+ if (ioctl(fd, FIOQSIZE, &sz) < 0)
+ return errno;
+ return 0;
+}
+
+static int test_tcgets_ioctl(int fd)
+{
+ struct termios info;
+
+ if (ioctl(fd, TCGETS, &info) < 0)
+ return errno;
+ return 0;
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_file)
+{
+ const int flag = 0;
+ const struct rule rules[] = {
+ {
+ .path = "/dev",
+ .access = variant->allowed,
+ },
+ {},
+ };
+ int file_fd, ruleset_fd;
+
+ /* Enables Landlock. */
+ ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ file_fd = open("/dev/tty", variant->open_mode);
+ ASSERT_LE(0, file_fd);
+
+ /* Checks that IOCTL commands return the expected errors. */
+ EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+ EXPECT_EQ(variant->expected_fionread_result,
+ test_fionread_ioctl(file_fd));
+
+ /* Checks that unrestrictable commands are unrestricted. */
+ EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+ EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+ EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+ EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+ EXPECT_EQ(ENOTTY, test_fioqsize_ioctl(file_fd));
+
+ ASSERT_EQ(0, close(file_fd));
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_dir)
+{
+ const int flag = 0;
+ const struct rule rules[] = {
+ {
+ .path = "/dev",
+ .access = variant->allowed,
+ },
+ {},
+ };
+ int dir_fd, ruleset_fd;
+
+ /* Enables Landlock. */
+ ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Ignore variant->open_mode for this test, as we intend to open a
+ * directory. If the directory can not be opened, the variant is
+ * infeasible to test with an opened directory.
+ */
+ dir_fd = open("/dev", O_RDONLY);
+ if (dir_fd < 0)
+ return;
+
+ /*
+ * Checks that IOCTL commands return the expected errors.
+ * We do not use the expected values from the fixture here.
+ *
+ * When using IOCTL on a directory, no Landlock restrictions apply.
+ * TCGETS will fail anyway because it is not invoked on a TTY device.
+ */
+ EXPECT_EQ(ENOTTY, test_tcgets_ioctl(dir_fd));
+ EXPECT_EQ(0, test_fionread_ioctl(dir_fd));
+
+ /* Checks that unrestrictable commands are unrestricted. */
+ EXPECT_EQ(0, ioctl(dir_fd, FIOCLEX));
+ EXPECT_EQ(0, ioctl(dir_fd, FIONCLEX));
+ EXPECT_EQ(0, ioctl(dir_fd, FIONBIO, &flag));
+ EXPECT_EQ(0, ioctl(dir_fd, FIOASYNC, &flag));
+ EXPECT_EQ(0, test_fioqsize_ioctl(dir_fd));
+
+ ASSERT_EQ(0, close(dir_fd));
+}
+
+TEST_F_FORK(ioctl, handle_file_access_file)
+{
+ const int flag = 0;
+ const struct rule rules[] = {
+ {
+ .path = "/dev/tty0",
+ .access = variant->allowed,
+ },
+ {},
+ };
+ int file_fd, ruleset_fd;
+
+ if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
+ SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
+ "can not be granted on files");
+ }
+
+ /* Enables Landlock. */
+ ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ file_fd = open("/dev/tty0", variant->open_mode);
+ ASSERT_LE(0, file_fd)
+ {
+ TH_LOG("Failed to open /dev/tty0: %s", strerror(errno));
+ }
+
+ /* Checks that IOCTL commands return the expected errors. */
+ EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+ EXPECT_EQ(variant->expected_fionread_result,
+ test_fionread_ioctl(file_fd));
+
+ /* Checks that unrestrictable commands are unrestricted. */
+ EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+ EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+ EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+ EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+ EXPECT_EQ(ENOTTY, test_fioqsize_ioctl(file_fd));
+
+ ASSERT_EQ(0, close(file_fd));
+}
+
/* clang-format off */
FIXTURE(layout1_bind) {};
/* clang-format on */
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 4/9] selftests/landlock: Test IOCTL with memfds
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (2 preceding siblings ...)
2024-03-25 13:39 ` [PATCH v12 3/9] selftests/landlock: Test IOCTL support Günther Noack
@ 2024-03-25 13:39 ` 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
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:39 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
Because the LANDLOCK_ACCESS_FS_IOCTL_DEV right is associated with the
opened file during open(2), IOCTLs are supposed to work with files
which are opened by means other than open(2).
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/fs_test.c | 36 ++++++++++++++++------
1 file changed, 27 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 22229fe3e403..32a77757462b 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3850,20 +3850,38 @@ static int test_fs_ioc_getflags_ioctl(int fd)
return 0;
}
-TEST(memfd_ftruncate)
+TEST(memfd_ftruncate_and_ioctl)
{
- int fd;
-
- fd = memfd_create("name", MFD_CLOEXEC);
- ASSERT_LE(0, fd);
+ const struct landlock_ruleset_attr attr = {
+ .handled_access_fs = ACCESS_ALL,
+ };
+ int ruleset_fd, fd, i;
/*
- * Checks that ftruncate is permitted on file descriptors that are
- * created in ways other than open(2).
+ * We exercise the same test both with and without Landlock enabled, to
+ * ensure that it behaves the same in both cases.
*/
- EXPECT_EQ(0, test_ftruncate(fd));
+ for (i = 0; i < 2; i++) {
+ /* Creates a new memfd. */
+ fd = memfd_create("name", MFD_CLOEXEC);
+ ASSERT_LE(0, fd);
- ASSERT_EQ(0, close(fd));
+ /*
+ * Checks that operations associated with the opened file
+ * (ftruncate, ioctl) are permitted on file descriptors that are
+ * created in ways other than open(2).
+ */
+ EXPECT_EQ(0, test_ftruncate(fd));
+ EXPECT_EQ(0, test_fs_ioc_getflags_ioctl(fd));
+
+ ASSERT_EQ(0, close(fd));
+
+ /* Enables Landlock. */
+ ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+ }
}
static int test_fionread_ioctl(int fd)
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 5/9] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH)
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (3 preceding siblings ...)
2024-03-25 13:39 ` [PATCH v12 4/9] selftests/landlock: Test IOCTL with memfds Günther Noack
@ 2024-03-25 13:40 ` Günther Noack
2024-03-25 13:40 ` [PATCH v12 6/9] selftests/landlock: Test IOCTLs on named pipes Günther Noack
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:40 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
ioctl(2) and ftruncate(2) operations on files opened with O_PATH
should always return EBADF, independent of the
LANDLOCK_ACCESS_FS_TRUNCATE and LANDLOCK_ACCESS_FS_IOCTL_DEV access
rights in that file hierarchy.
Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/fs_test.c | 40 ++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 32a77757462b..dde4673e2df4 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3893,6 +3893,46 @@ static int test_fionread_ioctl(int fd)
return 0;
}
+TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
+{
+ const struct landlock_ruleset_attr attr = {
+ .handled_access_fs = ACCESS_ALL,
+ };
+ int ruleset_fd, fd;
+
+ /*
+ * Checks that for files opened with O_PATH, both ioctl(2) and
+ * ftruncate(2) yield EBADF, as it is documented in open(2) for the
+ * O_PATH flag.
+ */
+ fd = open(dir_s1d1, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, fd);
+
+ EXPECT_EQ(EBADF, test_ftruncate(fd));
+ EXPECT_EQ(EBADF, test_fs_ioc_getflags_ioctl(fd));
+
+ ASSERT_EQ(0, close(fd));
+
+ /* Enables Landlock. */
+ ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Checks that after enabling Landlock,
+ * - the file can still be opened with O_PATH
+ * - both ioctl and truncate still yield EBADF (not EACCES).
+ */
+ fd = open(dir_s1d1, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, fd);
+
+ EXPECT_EQ(EBADF, test_ftruncate(fd));
+ EXPECT_EQ(EBADF, test_fs_ioc_getflags_ioctl(fd));
+
+ ASSERT_EQ(0, close(fd));
+}
+
/* clang-format off */
FIXTURE(ioctl) {};
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 6/9] selftests/landlock: Test IOCTLs on named pipes
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (4 preceding siblings ...)
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 ` 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
` (2 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:40 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
Named pipes should behave like pipes created with pipe(2),
so we don't want to restrict IOCTLs on them.
Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/fs_test.c | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index dde4673e2df4..d3aaa343f6e4 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3933,6 +3933,49 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
ASSERT_EQ(0, close(fd));
}
+/*
+ * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
+ * because they are not character or block devices.
+ */
+TEST_F_FORK(layout1, named_pipe_ioctl)
+{
+ pid_t child_pid;
+ int fd, ruleset_fd;
+ const char *const path = file1_s1d1;
+ const struct landlock_ruleset_attr attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+ };
+
+ ASSERT_EQ(0, unlink(path));
+ ASSERT_EQ(0, mkfifo(path, 0600));
+
+ /* Enables Landlock. */
+ ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* The child process opens the pipe for writing. */
+ child_pid = fork();
+ ASSERT_NE(-1, child_pid);
+ if (child_pid == 0) {
+ fd = open(path, O_WRONLY);
+ close(fd);
+ exit(0);
+ }
+
+ fd = open(path, O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ /* FIONREAD is implemented by pipefifo_fops. */
+ EXPECT_EQ(0, test_fionread_ioctl(fd));
+
+ ASSERT_EQ(0, close(fd));
+ ASSERT_EQ(0, unlink(path));
+
+ ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
+}
+
/* clang-format off */
FIXTURE(ioctl) {};
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 7/9] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (5 preceding siblings ...)
2024-03-25 13:40 ` [PATCH v12 6/9] selftests/landlock: Test IOCTLs on named pipes Günther Noack
@ 2024-03-25 13:40 ` 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
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:40 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/fs_test.c | 51 ++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d3aaa343f6e4..2ade195bde56 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -20,8 +20,10 @@
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/sendfile.h>
+#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/sysmacros.h>
+#include <sys/un.h>
#include <sys/vfs.h>
#include <unistd.h>
@@ -3976,6 +3978,55 @@ TEST_F_FORK(layout1, named_pipe_ioctl)
ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
}
+/* For named UNIX domain sockets, no IOCTL restrictions apply. */
+TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
+{
+ const char *const path = file1_s1d1;
+ int srv_fd, cli_fd, ruleset_fd;
+ socklen_t size;
+ struct sockaddr_un srv_un, cli_un;
+ const struct landlock_ruleset_attr attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+ };
+
+ /* Sets up a server */
+ srv_un.sun_family = AF_UNIX;
+ strncpy(srv_un.sun_path, path, sizeof(srv_un.sun_path));
+
+ ASSERT_EQ(0, unlink(path));
+ ASSERT_LE(0, (srv_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
+
+ size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
+ ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
+ ASSERT_EQ(0, listen(srv_fd, 10 /* qlen */));
+
+ /* Enables Landlock. */
+ ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Sets up a client connection to it */
+ cli_un.sun_family = AF_UNIX;
+
+ ASSERT_LE(0, (cli_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
+
+ size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
+ ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
+
+ bzero(&cli_un, sizeof(cli_un));
+ cli_un.sun_family = AF_UNIX;
+ strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
+ size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
+
+ ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
+
+ /* FIONREAD and other IOCTLs should not be forbidden. */
+ EXPECT_EQ(0, test_fionread_ioctl(cli_fd));
+
+ ASSERT_EQ(0, close(cli_fd));
+}
+
/* clang-format off */
FIXTURE(ioctl) {};
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 8/9] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (6 preceding siblings ...)
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 ` Günther Noack
2024-03-25 13:40 ` [PATCH v12 9/9] landlock: Document IOCTL support Günther Noack
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:40 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
Add IOCTL support to the Landlock sample tool.
The IOCTL right is grouped with the read-write rights in the sample
tool, as some IOCTL requests provide features that mutate state.
Signed-off-by: Günther Noack <gnoack@google.com>
---
samples/landlock/sandboxer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 32e930c853bb..997f774e88ae 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -81,7 +81,8 @@ static int parse_path(char *env_path, const char ***const path_list)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_IOCTL_DEV)
/* clang-format on */
@@ -201,11 +202,12 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
LANDLOCK_ACCESS_FS_MAKE_SYM | \
LANDLOCK_ACCESS_FS_REFER | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_IOCTL_DEV)
/* clang-format on */
-#define LANDLOCK_ABI_LAST 4
+#define LANDLOCK_ABI_LAST 5
int main(const int argc, char *const argv[], char *const *const envp)
{
@@ -319,6 +321,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
ruleset_attr.handled_access_net &=
~(LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP);
+ __attribute__((fallthrough));
+ case 4:
+ /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
+ ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
fprintf(stderr,
"Hint: You should update the running kernel "
"to leverage Landlock features "
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 9/9] landlock: Document IOCTL support
2024-03-25 13:39 [PATCH v12 0/9] Landlock: IOCTL support Günther Noack
` (7 preceding siblings ...)
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 ` Günther Noack
8 siblings, 0 replies; 19+ messages in thread
From: Günther Noack @ 2024-03-25 13:40 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jeff Xu, Arnd Bergmann, Jorge Lucangeli Obes, Allen Webb,
Dmitry Torokhov, Paul Moore, Konstantin Meskhidze, Matt Bobrowski,
linux-fsdevel, Günther Noack
In the paragraph above the fallback logic, use the shorter phrasing
from the landlock(7) man page.
Signed-off-by: Günther Noack <gnoack@google.com>
---
Documentation/userspace-api/landlock.rst | 76 +++++++++++++++++++-----
1 file changed, 61 insertions(+), 15 deletions(-)
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 9dd636aaa829..145bd869c684 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -76,7 +76,8 @@ to be explicit about the denied-by-default access rights.
LANDLOCK_ACCESS_FS_MAKE_BLOCK |
LANDLOCK_ACCESS_FS_MAKE_SYM |
LANDLOCK_ACCESS_FS_REFER |
- LANDLOCK_ACCESS_FS_TRUNCATE,
+ LANDLOCK_ACCESS_FS_TRUNCATE |
+ LANDLOCK_ACCESS_FS_IOCTL_DEV,
.handled_access_net =
LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP,
@@ -85,10 +86,10 @@ to be explicit about the denied-by-default access rights.
Because we may not know on which kernel version an application will be
executed, it is safer to follow a best-effort security approach. Indeed, we
should try to protect users as much as possible whatever the kernel they are
-using. To avoid binary enforcement (i.e. either all security features or
-none), we can leverage a dedicated Landlock command to get the current version
-of the Landlock ABI and adapt the handled accesses. Let's check if we should
-remove access rights which are only supported in higher versions of the ABI.
+using.
+
+To be compatible with older Linux versions, we detect the available Landlock ABI
+version, and only use the available subset of access rights:
.. code-block:: c
@@ -114,6 +115,10 @@ remove access rights which are only supported in higher versions of the ABI.
ruleset_attr.handled_access_net &=
~(LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP);
+ __attribute__((fallthrough));
+ case 4:
+ /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
+ ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
}
This enables to create an inclusive ruleset that will contain our rules.
@@ -225,6 +230,7 @@ access rights per directory enables to change the location of such directory
without relying on the destination directory access rights (except those that
are required for this operation, see ``LANDLOCK_ACCESS_FS_REFER``
documentation).
+
Having self-sufficient hierarchies also helps to tighten the required access
rights to the minimal set of data. This also helps avoid sinkhole directories,
i.e. directories where data can be linked to but not linked from. However,
@@ -318,18 +324,26 @@ It should also be noted that truncating files does not require the
system call, this can also be done through :manpage:`open(2)` with the flags
``O_RDONLY | O_TRUNC``.
-When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
-right is associated with the newly created file descriptor and will be used for
-subsequent truncation attempts using :manpage:`ftruncate(2)`. The behavior is
-similar to opening a file for reading or writing, where permissions are checked
-during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
+The truncate right is associated with the opened file (see below).
+
+Rights associated with file descriptors
+---------------------------------------
+
+When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` and
+``LANDLOCK_ACCESS_FS_IOCTL_DEV`` rights is associated with the newly created
+file descriptor and will be used for subsequent truncation and ioctl attempts
+using :manpage:`ftruncate(2)` and :manpage:`ioctl(2)`. The behavior is similar
+to opening a file for reading or writing, where permissions are checked during
+:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
:manpage:`write(2)` calls.
-As a consequence, it is possible to have multiple open file descriptors for the
-same file, where one grants the right to truncate the file and the other does
-not. It is also possible to pass such file descriptors between processes,
-keeping their Landlock properties, even when these processes do not have an
-enforced Landlock ruleset.
+As a consequence, it is possible that a process has multiple open file
+descriptors referring to the same file, but Landlock enforces different things
+when operating with these file descriptors. This can happen when a Landlock
+ruleset gets enforced and the process keeps file descriptors which were opened
+both before and after the enforcement. It is also possible to pass such file
+descriptors between processes, keeping their Landlock properties, even when some
+of the involved processes do not have an enforced Landlock ruleset.
Compatibility
=============
@@ -458,6 +472,28 @@ Memory usage
Kernel memory allocated to create rulesets is accounted and can be restricted
by the Documentation/admin-guide/cgroup-v1/memory.rst.
+IOCTL support
+-------------
+
+The ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right restricts the use of
+:manpage:`ioctl(2)`, but it only applies to *newly opened* device files. This
+means specifically that pre-existing file descriptors like stdin, stdout and
+stderr are unaffected.
+
+Users should be aware that TTY devices have traditionally permitted to control
+other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL
+commands. Both of these require ``CAP_SYS_ADMIN`` on modern Linux systems, but
+the behavior is configurable for ``TIOCSTI``.
+
+On older systems, it is therefore recommended to close inherited TTY file
+descriptors, or to reopen them from ``/proc/self/fd/*`` without the
+``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right, if possible.
+
+Landlock's IOCTL support is coarse-grained at the moment, but may become more
+fine-grained in the future. Until then, users are advised to establish the
+guarantees that they need through the file hierarchy, by only allowing the
+``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right on files where it is really required.
+
Previous limitations
====================
@@ -495,6 +531,16 @@ bind and connect actions to only a set of allowed ports thanks to the new
``LANDLOCK_ACCESS_NET_BIND_TCP`` and ``LANDLOCK_ACCESS_NET_CONNECT_TCP``
access rights.
+IOCTL (ABI < 5)
+---------------
+
+IOCTL operations could not be denied before the fifth Landlock ABI, so
+:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an
+earlier ABI.
+
+Starting with the Landlock ABI version 5, it is possible to restrict the use of
+:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` access right.
+
.. _kernel_support:
Kernel support
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 19+ messages in thread