| Message ID | 20240812174421.1636724-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] fs,security: Fix file_set_fowner LSM hook inconsistencies | expand |
On Mon, Aug 12, 2024 at 07:44:17PM +0200, Mickaël Salaün wrote: No opinion about the core idea, I'll note though that this conflicts with a patch to move f_owner out of the struct: https://lore.kernel.org/linux-fsdevel/20240809-koriander-biobauer-6237cbc106f3@brauner/ Presumably nothing which can't get sorted out with some shoveling. I do have actionable remark concerning creds though: both get_cred and put_cred are slow. Sorting that out is on my todo list. In the meantime adding more calls can be avoided: > diff --git a/fs/file_table.c b/fs/file_table.c > index 4f03beed4737..d28b76aef4f3 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -66,6 +66,7 @@ static inline void file_free(struct file *f) > if (likely(!(f->f_mode & FMODE_NOACCOUNT))) > percpu_counter_dec(&nr_files); > put_cred(f->f_cred); > + put_cred(f->f_owner.cred); if (likely(f->f_cred == f->f_owner.cred)) { put_cred_many(f->f_cred, 2); } else { put_cred(f->f_cred); put_cred(f->f_owner.cred); } > if (unlikely(f->f_mode & FMODE_BACKING)) { > path_put(backing_file_user_path(f)); > kfree(backing_file(f)); > @@ -149,9 +150,11 @@ static int init_file(struct file *f, int flags, const struct cred *cred) > int error; > > f->f_cred = get_cred(cred); > + f->f_owner.cred = get_cred(cred); f->f_cred = f->f_owner.cred = get_cred_many(cred, 2); > error = security_file_alloc(f); > if (unlikely(error)) { > put_cred(f->f_cred); > + put_cred(f->f_owner.cred); put_cred_many(cred, 2); > return error; > }
On Mon, Aug 12, 2024 at 1:44 PM Mickaël Salaün <mic@digikod.net> 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@zeniv.linux.org.uk> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Cc: Christian Brauner <brauner@kernel.org> > Cc: James Morris <jmorris@namei.org> > Cc: Jann Horn <jannh@google.com> > Cc: Ondrej Mosnacek <omosnace@redhat.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > > Changes since v1: > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > - 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. 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 ;)
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@digikod.net> 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@zeniv.linux.org.uk> > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: James Morris <jmorris@namei.org> > > Cc: Jann Horn <jannh@google.com> > > Cc: Ondrej Mosnacek <omosnace@redhat.com> > > Cc: Paul Moore <paul@paul-moore.com> > > Cc: Serge E. Hallyn <serge@hallyn.com> > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > --- > > > > Changes since v1: > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > > - 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 why the v1 entirely removed the LSM hook, which is now useless. Also, f_modown() is renamed to __f_setown(). > > 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, it saves memory, it fixes the LSM/VFS inconsistency, it guarantees that the VFS semantic is always visible to each LSMs thanks to the use of the same f_owner.cred, and it avoids LSM mistakes (except if an LSM implements the now-useless hook).
On Mon, Aug 12, 2024 at 11:45:29PM +0200, Mateusz Guzik wrote: > On Mon, Aug 12, 2024 at 07:44:17PM +0200, Mickaël Salaün wrote: > > No opinion about the core idea, I'll note though that this conflicts > with a patch to move f_owner out of the struct: > https://lore.kernel.org/linux-fsdevel/20240809-koriander-biobauer-6237cbc106f3@brauner/ Thanks for the heads-up. > > Presumably nothing which can't get sorted out with some shoveling. > > I do have actionable remark concerning creds though: both get_cred and > put_cred are slow. Sorting that out is on my todo list. > > In the meantime adding more calls can be avoided: OK, I'll add that in the next version. > > > diff --git a/fs/file_table.c b/fs/file_table.c > > index 4f03beed4737..d28b76aef4f3 100644 > > --- a/fs/file_table.c > > +++ b/fs/file_table.c > > @@ -66,6 +66,7 @@ static inline void file_free(struct file *f) > > if (likely(!(f->f_mode & FMODE_NOACCOUNT))) > > percpu_counter_dec(&nr_files); > > put_cred(f->f_cred); > > + put_cred(f->f_owner.cred); > > if (likely(f->f_cred == f->f_owner.cred)) { > put_cred_many(f->f_cred, 2); > } else { > put_cred(f->f_cred); > put_cred(f->f_owner.cred); > } > > > if (unlikely(f->f_mode & FMODE_BACKING)) { > > path_put(backing_file_user_path(f)); > > kfree(backing_file(f)); > > @@ -149,9 +150,11 @@ static int init_file(struct file *f, int flags, const struct cred *cred) > > int error; > > > > f->f_cred = get_cred(cred); > > + f->f_owner.cred = get_cred(cred); > > f->f_cred = f->f_owner.cred = get_cred_many(cred, 2); > > > error = security_file_alloc(f); > > if (unlikely(error)) { > > put_cred(f->f_cred); > > + put_cred(f->f_owner.cred); > > put_cred_many(cred, 2); > > > return error; > > } >
On Tue, Aug 13, 2024 at 6:05 AM Mickaël Salaün <mic@digikod.net> 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@digikod.net> 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@zeniv.linux.org.uk> > > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > > Cc: Christian Brauner <brauner@kernel.org> > > > Cc: James Morris <jmorris@namei.org> > > > Cc: Jann Horn <jannh@google.com> > > > Cc: Ondrej Mosnacek <omosnace@redhat.com> > > > Cc: Paul Moore <paul@paul-moore.com> > > > Cc: Serge E. Hallyn <serge@hallyn.com> > > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > --- > > > > > > Changes since v1: > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > > > - 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. 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. > > 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. > it saves memory 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. > 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? > 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. > 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.
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@digikod.net> 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@digikod.net> 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@zeniv.linux.org.uk> > > > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > > > Cc: Christian Brauner <brauner@kernel.org> > > > > Cc: James Morris <jmorris@namei.org> > > > > Cc: Jann Horn <jannh@google.com> > > > > Cc: Ondrej Mosnacek <omosnace@redhat.com> > > > > Cc: Paul Moore <paul@paul-moore.com> > > > > Cc: Serge E. Hallyn <serge@hallyn.com> > > > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > > --- > > > > > > > > Changes since v1: > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > > > > - 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. > > 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. > > > > 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. > > > it saves memory > > 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. > > > 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? > > > 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? > > -- > paul-moore.com
On Tue, Aug 13, 2024 at 2:28 PM Mickaël Salaün <mic@digikod.net> 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@digikod.net> 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@digikod.net> 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@zeniv.linux.org.uk> > > > > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > > > > Cc: Christian Brauner <brauner@kernel.org> > > > > > Cc: James Morris <jmorris@namei.org> > > > > > Cc: Jann Horn <jannh@google.com> > > > > > Cc: Ondrej Mosnacek <omosnace@redhat.com> > > > > > Cc: Paul Moore <paul@paul-moore.com> > > > > > Cc: Serge E. Hallyn <serge@hallyn.com> > > > > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > > > --- > > > > > > > > > > Changes since v1: > > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > > > > > - 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. > > 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. 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 ;) ... 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. > > > 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. > > > 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.
On Wed, Aug 14, 2024 at 1:39 AM Paul Moore <paul@paul-moore.com> wrote: > > 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. > The patch as posted here adds 2 atomics in the fast path and that indeed is a problem, but it can be trivially avoided -- either use get/put_cred_many or make it so that the same pointer means the ref is held implicitly (after all the f_cred one is guaranteed to be there for the entire file's lifetime). Either way extra overhead does not have to be there (modulo one branch on teardown to check for mismatched creds) and can be considered a non-factor. I have no basis to comment on the idea behind the patch. I'll note however that the patch to move f_owner out of struct file (and have *not* present by default) is likely to come through, it already landed here: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc&id=0e8540d012189259261c75360d2725a2107761e7 I don't know if it has any bearing on viability of the patch posted here.
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@digikod.net> 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@digikod.net> 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@digikod.net> 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@zeniv.linux.org.uk> > > > > > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > > > > > Cc: Christian Brauner <brauner@kernel.org> > > > > > > Cc: James Morris <jmorris@namei.org> > > > > > > Cc: Jann Horn <jannh@google.com> > > > > > > Cc: Ondrej Mosnacek <omosnace@redhat.com> > > > > > > Cc: Paul Moore <paul@paul-moore.com> > > > > > > Cc: Serge E. Hallyn <serge@hallyn.com> > > > > > > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] > > > > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] > > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > > > > --- > > > > > > > > > > > > Changes since v1: > > > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net > > > > > > - 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 >
On Wed, Aug 14, 2024 at 8:35 AM Mickaël Salaün <mic@digikod.net> wrote: > 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@digikod.net> 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@digikod.net> 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@digikod.net> wrote: ... > > > > > 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). Thanks for the clarification. I believe moving the hook as discussed should resolve this too.
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); } - -void __f_setown(struct file *filp, struct pid *pid, enum pid_type type, - int force) -{ - security_file_set_fowner(filp); - f_modown(filp, pid, type, force); -} EXPORT_SYMBOL(__f_setown); int f_setown(struct file *filp, int who, int force) @@ -146,7 +141,7 @@ EXPORT_SYMBOL(f_setown); void f_delown(struct file *filp) { - f_modown(filp, NULL, PIDTYPE_TGID, 1); + __f_setown(filp, NULL, PIDTYPE_TGID, 1); } pid_t f_getown(struct file *filp) @@ -249,13 +244,15 @@ static int f_getowner_uids(struct file *filp, unsigned long arg) { struct user_namespace *user_ns = current_user_ns(); uid_t __user *dst = (void __user *)arg; + const struct cred *fown_cred; uid_t src[2]; int err; - read_lock_irq(&filp->f_owner.lock); - src[0] = from_kuid(user_ns, filp->f_owner.uid); - src[1] = from_kuid(user_ns, filp->f_owner.euid); - read_unlock_irq(&filp->f_owner.lock); + rcu_read_lock(); + fown_cred = rcu_dereference(filp->f_owner->cred); + src[0] = from_kuid(user_ns, fown_cred->uid); + src[1] = from_kuid(user_ns, fown_cred->euid); + rcu_read_unlock(); err = put_user(src[0], &dst[0]); err |= put_user(src[1], &dst[1]); @@ -737,14 +734,17 @@ static const __poll_t band_table[NSIGPOLL] = { static inline int sigio_perm(struct task_struct *p, struct fown_struct *fown, int sig) { - const struct cred *cred; + const struct cred *cred, *fown_cred; int ret; rcu_read_lock(); + fown_cred = rcu_dereference(fown->cred); cred = __task_cred(p); - ret = ((uid_eq(fown->euid, GLOBAL_ROOT_UID) || - uid_eq(fown->euid, cred->suid) || uid_eq(fown->euid, cred->uid) || - uid_eq(fown->uid, cred->suid) || uid_eq(fown->uid, cred->uid)) && + ret = ((uid_eq(fown_cred->euid, GLOBAL_ROOT_UID) || + uid_eq(fown_cred->euid, cred->suid) || + uid_eq(fown_cred->euid, cred->uid) || + uid_eq(fown_cred->uid, cred->suid) || + uid_eq(fown_cred->uid, cred->uid)) && !security_file_send_sigiotask(p, fown, sig)); rcu_read_unlock(); return ret; diff --git a/fs/file_table.c b/fs/file_table.c index 4f03beed4737..d28b76aef4f3 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -66,6 +66,7 @@ static inline void file_free(struct file *f) if (likely(!(f->f_mode & FMODE_NOACCOUNT))) percpu_counter_dec(&nr_files); put_cred(f->f_cred); + put_cred(f->f_owner.cred); if (unlikely(f->f_mode & FMODE_BACKING)) { path_put(backing_file_user_path(f)); kfree(backing_file(f)); @@ -149,9 +150,11 @@ static int init_file(struct file *f, int flags, const struct cred *cred) int error; f->f_cred = get_cred(cred); + f->f_owner.cred = get_cred(cred); error = security_file_alloc(f); if (unlikely(error)) { put_cred(f->f_cred); + put_cred(f->f_owner.cred); return error; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 0283cf366c2a..345e8ff6d49a 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -942,7 +942,7 @@ struct fown_struct { rwlock_t lock; /* protects pid, uid, euid fields */ struct pid *pid; /* pid or -pgrp where SIGIO should be sent */ enum pid_type pid_type; /* Kind of process group SIGIO should be sent to */ - kuid_t uid, euid; /* uid/euid of process setting the owner */ + const struct cred __rcu *cred;/* cred of process setting the owner */ int signum; /* posix.1b rt signal to be delivered on IO */ }; diff --git a/security/security.c b/security/security.c index e5ca08789f74..6b2b1b56333c 100644 --- a/security/security.c +++ b/security/security.c @@ -2903,7 +2903,10 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg) * @file: the file * * Save owner security information (typically from current->security) in - * file->f_security for later use by the send_sigiotask hook. + * file->f_security for later use by the send_sigiotask hook. We should use + * fown_struct.cred instead though. + * + * This hook is called while the caller is locking fown_struct.lock . * * Return: Returns 0 on success. */ diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7eed331e90f0..a8f5ed66808d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3644,8 +3644,6 @@ static int selinux_file_alloc_security(struct file *file) u32 sid = current_sid(); fsec->sid = sid; - fsec->fown_sid = sid; - return 0; } @@ -3918,33 +3916,20 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd, return err; } -static void selinux_file_set_fowner(struct file *file) -{ - struct file_security_struct *fsec; - - fsec = selinux_file(file); - fsec->fown_sid = current_sid(); -} - static int selinux_file_send_sigiotask(struct task_struct *tsk, struct fown_struct *fown, int signum) { - struct file *file; u32 sid = task_sid_obj(tsk); u32 perm; - struct file_security_struct *fsec; - - /* struct fown_struct is never outside the context of a struct file */ - file = container_of(fown, struct file, f_owner); - - fsec = selinux_file(file); + const struct task_security_struct *tsec = + selinux_cred(rcu_dereference(fown->cred)); if (!signum) perm = signal_to_av(SIGIO); /* as per send_sigio_to_task */ else perm = signal_to_av(signum); - return avc_has_perm(fsec->fown_sid, sid, + return avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS, perm, NULL); } @@ -7202,7 +7187,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = { LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect), LSM_HOOK_INIT(file_lock, selinux_file_lock), LSM_HOOK_INIT(file_fcntl, selinux_file_fcntl), - LSM_HOOK_INIT(file_set_fowner, selinux_file_set_fowner), LSM_HOOK_INIT(file_send_sigiotask, selinux_file_send_sigiotask), LSM_HOOK_INIT(file_receive, selinux_file_receive), diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index dea1d6f3ed2d..d55b7f8d3a3d 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -56,7 +56,6 @@ struct inode_security_struct { struct file_security_struct { u32 sid; /* SID of open file description */ - u32 fown_sid; /* SID of file owner (for SIGIO) */ u32 isid; /* SID of inode at the time of file open */ u32 pseqno; /* Policy seqno at the time of file open */ }; diff --git a/security/smack/smack.h b/security/smack/smack.h index 041688e5a77a..06bac00cc796 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -328,12 +328,6 @@ static inline struct task_smack *smack_cred(const struct cred *cred) return cred->security + smack_blob_sizes.lbs_cred; } -static inline struct smack_known **smack_file(const struct file *file) -{ - return (struct smack_known **)(file->f_security + - smack_blob_sizes.lbs_file); -} - static inline struct inode_smack *smack_inode(const struct inode *inode) { return inode->i_security + smack_blob_sizes.lbs_inode; diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index f5cbec1e6a92..280a3da4c232 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1650,26 +1650,6 @@ static void smack_inode_getsecid(struct inode *inode, u32 *secid) * label changing that SELinux does. */ -/** - * smack_file_alloc_security - assign a file security blob - * @file: the object - * - * The security blob for a file is a pointer to the master - * label list, so no allocation is done. - * - * f_security is the owner security information. It - * isn't used on file access checks, it's for send_sigio. - * - * Returns 0 - */ -static int smack_file_alloc_security(struct file *file) -{ - struct smack_known **blob = smack_file(file); - - *blob = smk_of_current(); - return 0; -} - /** * smack_file_ioctl - Smack check on ioctls * @file: the object @@ -1888,18 +1868,6 @@ static int smack_mmap_file(struct file *file, return rc; } -/** - * smack_file_set_fowner - set the file security blob value - * @file: object in question - * - */ -static void smack_file_set_fowner(struct file *file) -{ - struct smack_known **blob = smack_file(file); - - *blob = smk_of_current(); -} - /** * smack_file_send_sigiotask - Smack on sigio * @tsk: The target task @@ -1914,7 +1882,6 @@ static void smack_file_set_fowner(struct file *file) static int smack_file_send_sigiotask(struct task_struct *tsk, struct fown_struct *fown, int signum) { - struct smack_known **blob; struct smack_known *skp; struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred)); const struct cred *tcred; @@ -1928,8 +1895,7 @@ static int smack_file_send_sigiotask(struct task_struct *tsk, file = container_of(fown, struct file, f_owner); /* we don't log here as rc can be overriden */ - blob = smack_file(file); - skp = *blob; + skp = smk_of_task(smack_cred(rcu_dereference(fown->cred))); rc = smk_access(skp, tkp, MAY_DELIVER, NULL); rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc); @@ -5014,7 +4980,6 @@ static int smack_uring_cmd(struct io_uring_cmd *ioucmd) struct lsm_blob_sizes smack_blob_sizes __ro_after_init = { .lbs_cred = sizeof(struct task_smack), - .lbs_file = sizeof(struct smack_known *), .lbs_inode = sizeof(struct inode_smack), .lbs_ipc = sizeof(struct smack_known *), .lbs_msg_msg = sizeof(struct smack_known *), @@ -5065,14 +5030,12 @@ static struct security_hook_list smack_hooks[] __ro_after_init = { LSM_HOOK_INIT(inode_listsecurity, smack_inode_listsecurity), LSM_HOOK_INIT(inode_getsecid, smack_inode_getsecid), - LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security), LSM_HOOK_INIT(file_ioctl, smack_file_ioctl), LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl), LSM_HOOK_INIT(file_lock, smack_file_lock), LSM_HOOK_INIT(file_fcntl, smack_file_fcntl), LSM_HOOK_INIT(mmap_file, smack_mmap_file), LSM_HOOK_INIT(mmap_addr, cap_mmap_addr), - LSM_HOOK_INIT(file_set_fowner, smack_file_set_fowner), LSM_HOOK_INIT(file_send_sigiotask, smack_file_send_sigiotask), LSM_HOOK_INIT(file_receive, smack_file_receive),
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@zeniv.linux.org.uk> Cc: Casey Schaufler <casey@schaufler-ca.com> Cc: Christian Brauner <brauner@kernel.org> Cc: James Morris <jmorris@namei.org> Cc: Jann Horn <jannh@google.com> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Serge E. Hallyn <serge@hallyn.com> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1] Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2] Signed-off-by: Mickaël Salaün <mic@digikod.net> --- Changes since v1: https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net - 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(-)