On Fri, May 31, 2024 at 3:32 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Fri, May 31, 2024 at 12:02:16PM +0200, Christian Brauner wrote: > > On Thu, May 30, 2024 at 08:49:12AM -0700, Linus Torvalds wrote: > > > On Thu, 30 May 2024 at 03:32, Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > > > Ofc depends on whether Linus still agrees that removing this might be > > > > something we could try. > > > > > > I _definitely_ do not want to see any more complex deny_write_access(). > > > > > > So yes, if people have good reasons to override the inode write > > > access, I'd rather remove it entirely than make it some eldritch > > > horror that is even worse than what we have now. > > > > > > It would obviously have to be tested in case some odd case actually > > > depends on the ETXTBSY semantics, since we *have* supported it for a > > > long time. But iirc nobody even noticed when we removed it from > > > shared libraries, so... > > > > > > That said, verity seems to depend on it as a way to do the > > > "enable_verity()" atomically with no concurrent writes, and I see some > > > i_writecount noise in the integrity code too. This one is a bit more challenging. The IMA ima_bprm_check() LSM hook (called from exec_binprm() context) may read the file (in ima_collect_measurement()) and record the signature of the file to be executed, assuming that it cannot be modified. Not sure how to deal with this expectation. Only thing I could think of is that IMA would be allowed to deny_write_access() and set FMODE_EXEC_DENY_WRITE as a hint for do_close_execat() to allow_write_access(), but it's pretty ugly, I admit. > > > > > > But maybe that's just a belt-and-suspenders thing? > > > > > > Because if execve() no longer does it, I think we should just remove > > > that i_writecount thing entirely. > > > > deny_write_access() is being used from kernel_read_file() which has a > > few wrappers around it and they are used in various places: > > > > (1) kernel_read_file() based helpers: > > (1.1) kernel_read_file_from_path() > > (1.2) kernel_read_file_from_path_initns() > > (1.3) kernel_read_file_from_fd() > > > > (2) kernel_read_file() users: > > (2.1) kernel/module/main.c:init_module_from_file() > > (2.2) security/loadpin/loadpin.c:read_trusted_verity_root_digests() > > > > (3) kernel_read_file_from_path() users: > > (3.1) security/integrity/digsig.c:integrity_load_x509() > > (3.2) security/integrity/ima/ima_fs.c:ima_read_busy() > > > > (4) kernel_read_file_from_path_initns() users: > > (4.1) drivers/base/firmware_loader/main.c:fw_get_filesystem_firmware() > > > > (5) kernel_read_file_from_fd() users: > > (5.1) kernel/kexec_file.c:kimage_file_prepare_segments() > > > > In order to remove i_writecount completely we would need to do this in > > Sorry, typo s/i_write_count/deny_write_access()/g > (I don't think we can remove i_writecount itself as it's used for file > leases and locks.) Indeed, i_writecount (as does i_readcount) is used by fs/locks.c: check_conflicting_open(), but not as a synchronization primitive. > > > multiple steps as some of that stuff seems potentially sensitive. > > > > The exec deny write mechanism can be removed because we have a decent > > understanding of the implications and there's decent justification for > > removing it. > > > > So I propose that I do various testing (LTP) etc. now, send the patch > > and then put this into -next to see if anything breaks? Wouldn't hurt to see what else we are missing. Thanks, Amir.
