On Tue, Nov 05, 2019 at 09:02:09AM -0800, Daniel Colascione wrote: > And you're suggesting making a security check work weirdly unlike most > other security checks because you hope it'll get removed one day? I didn't actually suggest that, I was only asking clarifications that I understood correctly because up until that point you didn't seem to say that the "permission check" needs to remain in UFFDIO_API. > Temporary solutions aren't, and if something goes into the kernel at > all, it's worth getting right. The general rule is that access checks > happen at open time. The kernel has already been bitten by UFFD > exempting itself from the normal rules (e.g., the > read(2)-makes-a-file-descriptor thing) in the name of expediency. > There shouldn't be any more exceptions. It didn't occur to me that not doing the measurement in the syscall that opens an fd is weird. The posted patch doesn't work any different than fscrypt_ioctl_add_key in FS_IOC_ADD_ENCRYPTION_KEY of ext4 and others, or btrfs_ioctl_fssetxattr or a ton of other examples where permissions are checked directly the in ioctl of the files and the measurement is also done in the ioctl and not in the open() as you suggest as the only non-weird solution that should exist in the kernel. I can surely provide a lot more examples of the exact same paradigm where the measurement of the capability is done in the ioctl, those are the first two examples that show up so it's unlikely they're the only ones. So overall I didn't think this was something wrong to do, or weird or something particularly new and I didn't look like we were bitting anything with it. And more than in the name of expediency this simply looks preferable to keep the complexity of the kernel low which in turns it means it's going to be more secure and simpler to maintain. Especially considering this code is likely to be modified later. Said that I've nothing contrary to do the more complex solution if that's the correct thing to do despite more complex and despite the code is pending for removal anyway, just I don't see any difference between the current simple patch to what ext4_ioctl does in FS_IOC_ADD_ENCRYPTION_KEY + FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR.
