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. 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. 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. -- paul-moore.com
