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

On Fri, Aug 9, 2024 at 10:01 AM Jann Horn [off-list ref] 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'm not entirely clear on what is being proposed either.  Some quick
pseudo code might do wonders here to help clarify things.

From a LSM perspective I suspect we are always going to need some sort
of hook in the F_SETOWN code path as the LSM needs to potentially
capture state/attributes/something-LSM-specific at that
context/point-in-time.  While I think it is okay if we want to
consider relocating the security_file_set_fowner() within the F_SETOWN
call path, I don't think we can remove it, even if we add additional
LSM security blobs.

-- 
paul-moore.com