On Fri, Apr 05, 2024 at 06:17:17PM +0200, Günther Noack wrote: > On Wed, Apr 03, 2024 at 01:15:45PM +0200, Mickaël Salaün wrote: > > On Tue, Apr 02, 2024 at 08:28:49PM +0200, Günther Noack wrote: > > > On Wed, Mar 27, 2024 at 05:57:31PM +0100, Mickaël Salaün wrote: > > > > On Wed, Mar 27, 2024 at 01:10:31PM +0000, Günther Noack wrote: > > > > > + case FIOQSIZE: > > > > > + /* > > > > > + * FIOQSIZE queries the size of a regular file or directory. > > > > > + * > > > > > + * This IOCTL command only applies to regular files and > > > > > + * directories. > > > > > + */ > > > > > + return LANDLOCK_ACCESS_FS_IOCTL_DEV; > > > > > > > > This should always be allowed because do_vfs_ioctl() never returns > > > > -ENOIOCTLCMD for this command. That's why I wrote > > > > vfs_masked_device_ioctl() this way [1]. I think it would be easier to > > > > read and maintain this code with a is_masked_device_ioctl() logic. Listing > > > > commands that are not masked makes it difficult to review because > > > > allowed and denied return codes are interleaved. > > > > > > Oh, I misunderstood you on [2], I think -- I was under the impression that you > > > wanted to keep the switch case in the same order (and with the same entries?) as > > > the original in do_vfs_ioctl. So you'd prefer to only list the always-allowed > > > IOCTL commands here, as you have done in vfs_masked_device_ioctl() [3]? > > > > > > [2] https://lore.kernel.org/all/20240326.ooCheem1biV2@xxxxxxxxxxx/ > > > [3] https://lore.kernel.org/all/20240219183539.2926165-1-mic@xxxxxxxxxxx/ > > > > That was indeed unclear. About IOCTL commands, the same order ease > > reviewing and maintenance but we don't need to list all commands, > > which will limit updates of this list. However, for the current > > unused/unmasked one, we can still add them very briefly in comments as I > > did with FIONREAD and file_ioctl()'s ones in vfs_masked_device_ioctl(). > > Only listing the "masked" ones (for device case) shorten the list, and > > having a list with the same semantic ("mask device-specific IOCTLs") > > ease review and maintenance as well. > > > > > > > > Can you please clarify how you make up your mind about what should be permitted > > > and what should not? I have trouble understanding the rationale for the changes > > > that you asked for below, apart from the points that they are harmless and that > > > the return codes should be consistent. > > > > The rationale is the same: all IOCTL commands that are not > > passed/specific to character or block devices (i.e. IOCTLs defined in > > fs/ioctl.c) are allowed. vfs_masked_device_ioctl() returns true if the > > IOCTL command is not passed to the related device driver but handled by > > fs/ioctl.c instead (i.e. handled by the VFS layer). > > Thanks for clarifying -- this makes more sense now. I traced the cases with > -ENOIOCTLCMD through the code more thoroughly and it is more aligned now with > what you implemented before. The places where I ended up implementing it > differently to your vfs_masked_device_ioctl() patch are: > > * Do not blanket-permit FS_IOC_{GET,SET}{FLAGS,XATTR}. > They fall back to the device implementation. > > * FS_IOC_GETUUID and FS_IOC_GETFSSYSFSPATH are now handled. > These return -ENOIOCTLCMD from do_vfs_ioctl(), so they do fall back to the > handlers in struct file_operations, so we can not permit these either. Good catch! > > These seem like pretty clear cases to me. > > > > > The criteria that I have used in this patch set are that (a) it is implemented > > > in do_vfs_ioctl() rather than further below, and (b) it makes sense to use that > > > command on a device file. (If we permit FIOQSIZE, FS_IOC_FIEMAP and others > > > here, we will get slightly more correct error codes in these cases, but the > > > IOCTLs will still not work, because they are not useful and not implemented for > > > devices. -- On the other hand, we are also increasing the exposed code surface a > > > bit. For example, FS_IOC_FIEMAP is calling into inode->i_op->fiemap(). That is > > > probably harmless for device files, but requires us to reason at a deeper level > > > to convince ourselves of that.) > > > > FIOQSIZE is fully handled by do_vfs_ioctl(), and FS_IOC_FIEMAP is > > implemented as the inode level, so it should not be passed at the struct > > file/device level unless ENOIOCTLCMD is returned (but it should not, > > right?). Because it depends on the inode implementation, it looks like > > this IOCTL may work (in theory) on character or block devices too. If > > this is correct, we should not deny it because the semantic of > > LANDLOCK_ACCESS_FS_IOCTL_DEV is to control IOCTLs passed to device > > drivers. Furthermore, as you pointed out, error codes would be > > unaltered. > > > > It would be good to test (as you suggested IIRC) the masked commands on > > a simple device (e.g. /dev/null) to check that it returns ENOTTY, > > EOPNOTSUPP, or EACCES according to our expectations. > > Sounds good, I'll add a test. > > > > I agree that this would increase a bit the exposed code surface but I'm > > pretty sure that if a sandboxed process is allowed to access a device > > file, it is also allowed to access directory or other file types as well > > and then would still be able to reach the FS_IOC_FIEMAP implementation. > > I assume you mean FIGETBSZ? The FS_IOC_FIEMAP IOCTL is the one that returns > file extent maps, so that user space can reason about whether a file is stored > in a consecutive way on disk. I meant FS_IOC_FIEMAP for regular files. > > > > I'd like to avoid exceptions as in the current implementation of > > get_required_ioctl_dev_access() with a switch/case either returning 0 or > > LANDLOCK_ACCESS_FS_IOCTL_DEV (excluding the default case of course). An > > alternative approach would be to group IOCTL command cases according to > > their returned value, but I find it a bit more complex for no meaningful > > gain. What do you think? > > I don't have strong opinions about it, as long as we don't accidentally mess up > the fallbacks if this changes. > > > > > In your implementation at [3], you were permitting FICLONE* and FIDEDUPERANGE, > > > but not FS_IOC_ZERO_RANGE, which is like fallocate(). How are these cases > > > different to each other? Is that on purpose? > > > > FICLONE* and FIDEDUPERANGE match device files and the > > vfs_clone_file_range()/generic_file_rw_checks() check returns EINVAL for > > device files. So there is no need to add exceptions for these commands. > > > > FS_IOC_ZERO_RANGE is only implemented for regular files (see > > file_ioctl() call), so it is passed to device files. > > Makes sense :) > > > —Günther >
