On Fri, May 31, 2024 at 04:09:17PM +0300, Amir Goldstein wrote: > 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. This is very annoying. And it probably doesn't work for dynamic binaries. 90% of the code will be in libraries to which one can happily write. Plus there's various attacks to break this: https://svs.informatik.uni-hamburg.de/publications/2020/2020-08-27-Bohling-IMA.pdf Anyway, I really really don't want to add more complex deny_write_access() either. It's hard to understand, it's hard to document and it punches holes into this anyway. The freezing levels are annoying enough already. So then I propose we just make the deny write stuff during exec conditional on IMA being active. At the end it's small- vs chicken pox. (I figure it won't be enough for IMA to read the executable after it has been mapped MS_PRIVATE?) > 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.
