Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

On Fri, Aug 09, 2024 at 04:00:41PM GMT, Jann Horn wrote:

On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün [off-list ref] wrote:

quoted

Talking about f_modown() and security_file_set_fowner(), it looks like
there are some issues:

On Fri, Aug 09, 2024 at 02:44:06PM +0200, Jann Horn wrote:

quoted

On Fri, Aug 9, 2024 at 12:59 PM Mickaël Salaün [off-list ref] wrote:

[...]

quoted

quoted

BTW, I don't understand why neither SELinux nor Smack use (explicit)
atomic operations nor lock.

Yeah, I think they're sloppy and kinda wrong - but it sorta works in
practice mostly because they don't have to do any refcounting around
this?

quoted

And it looks weird that
security_file_set_fowner() isn't called by f_modown() with the same
locking to avoid races.

True. I imagine maybe the thought behind this design could have been
that LSMs should have their own locking, and that calling an LSM hook
with IRQs off is a little weird? But the way the LSMs actually use the
hook now, it might make sense to call the LSM with the lock held and
IRQs off...

Would it be OK (for VFS, SELinux, and Smack maintainers) to move the
security_file_set_fowner() call into f_modown(), especially where
UID/EUID are populated.  That would only call security_file_set_fowner()
when the fown is actually set, which I think could also fix a bug for
SELinux and Smack.

Could we replace the uid and euid fields with a pointer to the current
credentials?  This would enables LSMs to not copy the same kind of
credential informations and save some memory, simplify credential
management, and improve consistency.

To clarify: These two paragraphs are supposed to be two alternative
options, right? One option is to call security_file_set_fowner() with
the lock held, the other option is to completely rip out the
security_file_set_fowner() hook and instead let the VFS provide LSMs
with the creds they need for the file_send_sigiotask hook?

I think it would be fine to stash the credentials in struct fown_struct
same as we do for struct file itself. Calling security_file_set_fowner()
with the irq lock held seems suboptimal to me. Plus, this also means one
less LSM hook and that seems like a win too.