On Thu, Mar 14, 2024 at 01:56:25PM -0400, Paul Moore wrote: > On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@xxxxxxxxxx> wrote: > > > > This LSM hook gets called just before the fs/ioctl.c logic delegates > > the requested IOCTL command to the file-specific implementation as > > implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat). > > > > It is impractical for LSMs to make security guarantees about these > > f_op operations without having intimate knowledge of how they are > > implemented. > > > > Therefore, depending on the enabled Landlock policy, Landlock aims to > > block the calls to filp->f_op->unlocked_ioctl(), but permit the calls > > to the IOCTL commands which are already implemented in fs/ioctl.c. > > > > The current call graph is: > > > > * ioctl syscall > > * security_file_ioctl() LSM hook > > * do_vfs_ioctl() - standard operations > > * file_ioctl() - standard file operations > > * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op) > > * filp->f_op->unlocked_ioctl() > > > > Why not use the existing security_file_ioctl() hook? > > > > With the existing security_file_ioctl() hook, it is technically > > feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it > > would be difficult to maintain: security_file_ioctl() gets called > > further up the call stack, so an implementation of it would need to > > predict whether the logic further below will decide to call > > f_op->unlocked_ioctl(). That can only be done by mirroring the logic > > in do_vfs_ioctl() to some extent, and keeping this in sync. > > Once again, I don't see this as an impossible task, and I would think > that you would want to inspect each new ioctl command/op added in > do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted > behavior from a Landlock sandbox perspective. About the LANDLOCK_ACCESS_FS_IOCTL_DEV semantic, we only care about the IOCTLs that are actually delivered to device drivers, so any new IOCTL directly handled by fs/ioctl.c should be treated the same way for this access right. > Looking at the git > log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are > added very frequently, meaning that keeping Landlock sync'd with > fs/ioctl.c shouldn't be a terrible task. do_vfs_ioctl() is indeed not changed often, but this doesn't mean we should not provide strong guarantees, avoid future security bugs, lower the maintenance cost, and improve code readability. > > I'm also not excited about the overlap between the existing > security_file_ioctl() hook and the proposed security_file_vfs_ioctl() > hook. There are some cases where we have no choice and we have to > tolerate the overlap, but this doesn't look like one of those cases to > me. > > I'm sorry, but I don't agree with this new hook. OK, I sent a new RFC (in reply to your email) as an alternative approach. Instead of adding a new LSM hook, this patch adds the vfs_get_ioctl_handler() helper and some code refactoring that should be both interesting for the VFS subsystem and for Landlock. I guess this could be interesting for other security mechanisms as well (e.g. BPF LSM). What do you think? Arnd, Christian, would this sound good to you?
