| Message ID | 20240325134004.4074874-2-gnoack@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
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
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:
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).
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
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.
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
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
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
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(-)
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)
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(-)