Re: [PATCH][RFC] fs: add levels to inode write access

[Amir Goldstein <amir73il@gmail.com>]

On Fri, May 31, 2024 at 3:32 PM Christian Brauner <brauner@kernel.org> 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@kernel.org> 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.