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.
