On Tue, Aug 13, 2024 at 07:39:45PM -0400, Paul Moore wrote: > On Tue, Aug 13, 2024 at 2:28 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > On Tue, Aug 13, 2024 at 11:04:13AM -0400, Paul Moore wrote: > > > On Tue, Aug 13, 2024 at 6:05 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > > > On Mon, Aug 12, 2024 at 06:26:58PM -0400, Paul Moore wrote: > > > > > On Mon, Aug 12, 2024 at 1:44 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > > > > > > > > > > > The fcntl's F_SETOWN command sets the process that handle SIGIO/SIGURG > > > > > > for the related file descriptor. Before this change, the > > > > > > file_set_fowner LSM hook was used to store this information. However, > > > > > > there are three issues with this approach: > > > > > > > > > > > > - Because security_file_set_fowner() only get one argument, all hook > > > > > > implementations ignore the VFS logic which may not actually change the > > > > > > process that handles SIGIO (e.g. TUN, TTY, dnotify). > > > > > > > > > > > > - Because security_file_set_fowner() is called before f_modown() without > > > > > > lock (e.g. f_owner.lock), concurrent F_SETOWN commands could result to > > > > > > a race condition and inconsistent LSM states (e.g. SELinux's fown_sid) > > > > > > compared to struct fown_struct's UID/EUID. > > > > > > > > > > > > - Because the current hook implementations does not use explicit atomic > > > > > > operations, they may create inconsistencies. It would help to > > > > > > completely remove this constraint, as well as the requirements of the > > > > > > RCU read-side critical section for the hook. > > > > > > > > > > > > Fix these issues by replacing f_owner.uid and f_owner.euid with a new > > > > > > f_owner.cred [1]. This also saves memory by removing dedicated LSM > > > > > > blobs, and simplifies code by removing file_set_fowner hook > > > > > > implementations for SELinux and Smack. > > > > > > > > > > > > This changes enables to remove the smack_file_alloc_security > > > > > > implementation, Smack's file blob, and SELinux's > > > > > > file_security_struct->fown_sid field. > > > > > > > > > > > > As for the UID/EUID, f_owner.cred is not always updated. Move the > > > > > > file_set_fowner hook to align with the VFS semantic. This hook does not > > > > > > have user anymore [2]. > > > > > > > > > > > > Before this change, f_owner's UID/EUID were initialized to zero > > > > > > (i.e. GLOBAL_ROOT_UID), but to simplify code, f_owner's cred is now > > > > > > initialized with the file descriptor creator's credentials (i.e. > > > > > > file->f_cred), which is more consistent and simplifies LSMs logic. The > > > > > > sigio_perm()'s semantic does not need any change because SIGIO/SIGURG > > > > > > are only sent when a process is explicitly set with __f_setown(). > > > > > > > > > > > > Rename f_modown() to __f_setown() to simplify code. > > > > > > > > > > > > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > > > > > > Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx> > > > > > > Cc: Christian Brauner <brauner@xxxxxxxxxx> > > > > > > Cc: James Morris <jmorris@xxxxxxxxx> > > > > > > Cc: Jann Horn <jannh@xxxxxxxxxx> > > > > > > Cc: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > > > > > > Cc: Paul Moore <paul@xxxxxxxxxxxxxx> > > > > > > Cc: Serge E. Hallyn <serge@xxxxxxxxxx> > > > > > > Cc: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > > > > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > > > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@xxxxxxxxxxxxxx [2] > > > > > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> > > > > > > --- > > > > > > > > > > > > Changes since v1: > > > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@xxxxxxxxxxx > > > > > > - Add back the file_set_fowner hook (but without user) as > > > > > > requested by Paul, but move it for consistency. > > > > > > --- > > > > > > fs/fcntl.c | 42 +++++++++++++++---------------- > > > > > > fs/file_table.c | 3 +++ > > > > > > include/linux/fs.h | 2 +- > > > > > > security/security.c | 5 +++- > > > > > > security/selinux/hooks.c | 22 +++------------- > > > > > > security/selinux/include/objsec.h | 1 - > > > > > > security/smack/smack.h | 6 ----- > > > > > > security/smack/smack_lsm.c | 39 +--------------------------- > > > > > > 8 files changed, 33 insertions(+), 87 deletions(-) > > > > > > > > > > > > diff --git a/fs/fcntl.c b/fs/fcntl.c > > > > > > index 300e5d9ad913..4217b66a4e99 100644 > > > > > > --- a/fs/fcntl.c > > > > > > +++ b/fs/fcntl.c > > > > > > @@ -87,8 +87,8 @@ static int setfl(int fd, struct file * filp, unsigned int arg) > > > > > > return error; > > > > > > } > > > > > > > > > > > > -static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, > > > > > > - int force) > > > > > > +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type, > > > > > > + int force) > > > > > > { > > > > > > write_lock_irq(&filp->f_owner.lock); > > > > > > if (force || !filp->f_owner.pid) { > > > > > > @@ -97,20 +97,15 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, > > > > > > filp->f_owner.pid_type = type; > > > > > > > > > > > > if (pid) { > > > > > > - const struct cred *cred = current_cred(); > > > > > > - filp->f_owner.uid = cred->uid; > > > > > > - filp->f_owner.euid = cred->euid; > > > > > > + security_file_set_fowner(filp); > > > > > > + put_cred(rcu_replace_pointer( > > > > > > + filp->f_owner.cred, > > > > > > + get_cred_rcu(current_cred()), > > > > > > + lockdep_is_held(&filp->f_owner.lock))); > > > > > > } > > > > > > } > > > > > > write_unlock_irq(&filp->f_owner.lock); > > > > > > } > > > > > > > > > > Looking at this quickly, why can't we accomplish pretty much the same > > > > > thing by moving the security_file_set_fowner() into f_modown (as > > > > > you've done above) and leveraging the existing file->f_security field > > > > > as Smack and SELinux do today? I'm seeing a lot of churn to get a > > > > > cred pointer into fown_struct which doesn't seem to offer that much > > > > > additional value. > > > > > > > > As explained in the commit message, this patch removes related LSM > > > > (sub)blobs because they are duplicates of what's referenced by the new > > > > f_owner.cred, which is a more generic approach and saves memory. > > > > > > That's not entirely correct. While yes you do remove the need for a > > > Smack entry in file->f_security, there is still a need for the SELinux > > > entry in file->f_security no matter what you do, and since the LSM > > > framework handles the LSM security blob allocations, on systems where > > > SELinux is enabled you are going to do a file->f_security allocation > > > regardless. > > > > That's why I used "(sub)" blob, for the case of SELinux that "only" drop > > a field. > > Your choice of phrasing was misleading in my opinion. > > > > While a cred based approach may be more generic from a traditional > > > UID/GID/etc. perspective, file->f_security is always going to be more > > > generic from a LSM perspective as the LSM has more flexibility about > > > what is placed into that blob. Yes, the LSM can also place data into > > > the cred struct, but that is used across a wide variety of kernel > > > objects and placing file specific data in there could needlessly > > > increase the size of the cred struct. > > > > Yes, it could, but that is not the case with the current implementations > > (SELinux and Smack). I understand that it could be useful though. > > Please keep that last sentence in mind. > > > > > > From what I can see this seems really focused on adding a cred > > > > > reference when it isn't clear an additional one is needed. If a new > > > > > cred reference *is* needed, please provide an explanation as to why; > > > > > reading the commit description this isn't clear. Of course, if I'm > > > > > mistaken, feel free to correct me, although I'm sure all the people on > > > > > the Internet don't need to be told that ;) > > > > > > > > This is a more generic approach that saves memory, sticks to the VFS > > > > semantic, and removes code. So I'd say it's a performance improvement > > > > > > Considering that additional cred gets/puts are needed I question if > > > there are actually any performance improvements; in some cases I > > > suspect the performance will actually be worse. On SELinux enabled > > > systems you are still going to do the file->f_security allocation and > > > now you are going to add the cred management operations on top of > > > that. > > > > I was talking about the extra hook calls which are not needed. The move > > of fown_struct ou of the file struct should limit any credential > > reference performance impact, and Mateusz said he is working on > > improving this part too. > > I don't see how where the cred reference live will have any impact, > you still need to get and drop references which will have an impact. > There will always be something. I meant that if there is no fown_struct data, there is no extra credential reference. > > > > With the move in linux-next to pull fown_struct out of the file > > > struct, I suspect this is not as important as it once may have been. > > > > I was talking about the LSM blobs shrinking, which impacts all opened > > files, independently of moving fown_struct out of the file struct. I > > think this is not negligible: 32 bits for SELinux + 64 bits for Smack + > > 64 bits for ongoing Landlock support = potentially 128 bits for each > > opened files. > > I'm going to skip over the Landlock contribution as that isn't part of > the patchset and to the best of my knowledge that is still a work in > progress and not finalized. > > You also forgot to add in the cost of the fown_struct->cred pointer. No because fown_struct.cred replaces fown_struct.uid and fown_struct.euid > > I noticed you chose to do your count in bits, likely to make the > numbers look bigger (which is fair), I'm going to do my count in bytes FWIW, I didn't write that with malice nor to make it look bigger. > ;) ... So we've got four bytes removed from the SELinux blob, and > eight bytes from the Smack blob, but we add back in another eight > bytes for the new cred pointer, making a net benefit of only four > bytes for each open file. Considering my concerns around the loss of > flexibility at the LSM layer I don't see this as a worthwhile > tradeoff. Considering that the uid and euid fields are removed, the net worth would be 12 bytes, but is is much more than that taking into account the move of fown_struct out of the file struct because the LSM blobs are per file, not per fown_struct. > > > > > it fixes the LSM/VFS inconsistency > > > > > > Simply moving the security_file_set_fowner() inside the lock protected > > > region should accomplish that too. Unless you're talking about > > > something else? > > > > Yes, the moving the hook fixes that. > > > > > > it guarantees > > > > that the VFS semantic is always visible to each LSMs thanks to the use > > > > of the same f_owner.cred > > > > > > The existing hooks are designed to make sure that the F_SETOWN > > > operation is visible to the LSM. > > > > This should not change the F_SETOWN case. Am I missing something? > > I don't know, you were talking about making sure the VFS semantics are > visible to the LSM and I was simply suggesting that the existing hooks > do that too. To be honest, whatever point you are trying to make here > isn't very clear to me. The existing hooks does not reflect the VFS semantic because of the `if (force || !filp->f_owner.pid)` checks in f_modown(). When f_modown() is explitly called from user space (F_SETOWN), force is true, but that is not the case for all call sites (e.g. TUN, TTY, dnotify). > > > > > and it avoids LSM mistakes (except if an LSM implements the now-useless hook). > > > > > > The only mistake I'm seeing is that the call into > > > security_file_set_fowner() is not in the lock protected region, and > > > that is easily corrected. Forcing the LSM framework to reuse a cred > > > struct has the potential to restrict LSM security models which is > > > something we try very hard not to do. > > > > OK, but is the current approach (i.e. keep the LSM hook and reducing LSM > > blobs size) good for you? What do you want me to remove from this > > patch? > > I agree that the security_file_set_owner() hook needs to be moved. I > disagree about the value in shifting the LSM framework over to a cred > reference which effectively abandons the existing hook. My preference > would be to preserve the flexibility of the hook, but move it to the > protected lock location, and continue to leverage the file->f_security > blob as needed for any LSM state. > > -- > paul-moore.com >
