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@xxxxxxxxxxx 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
