| Message ID | 20240821095609.365176-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] fs: Fix file_set_fowner LSM hook inconsistencies | expand |
On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which > may not actually change the process that handles SIGIO (e.g. TUN, TTY, > dnotify), nor update the related UID/EUID. > > Moreover, because security_file_set_fowner() was called 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. > > This change makes sure the LSM states are always in sync with the VFS > state by moving the security_file_set_fowner() call close to the > UID/EUID updates and using the same f_owner.lock . > > 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> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > > Changes since v2: > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net > - Only keep the LSM hook move. > > 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 | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) This looks reasonable to me, and fixes a potential problem with existing LSMs. Unless I hear any strong objections I'll plan to merge this, and patch 2/2, into the LSM tree tomorrow. > diff --git a/fs/fcntl.c b/fs/fcntl.c > index 300e5d9ad913..c28dc6c005f1 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) { > @@ -98,19 +98,13 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, > > if (pid) { > const struct cred *cred = current_cred(); > + security_file_set_fowner(filp); > filp->f_owner.uid = cred->uid; > filp->f_owner.euid = cred->euid; > } > } > 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 +140,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) > -- > 2.46.0
On Wed, Aug 21, 2024 at 12:32:17PM -0400, Paul Moore wrote: > On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which > > may not actually change the process that handles SIGIO (e.g. TUN, TTY, > > dnotify), nor update the related UID/EUID. > > > > Moreover, because security_file_set_fowner() was called 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. > > > > This change makes sure the LSM states are always in sync with the VFS > > state by moving the security_file_set_fowner() call close to the > > UID/EUID updates and using the same f_owner.lock . > > > > 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> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > --- > > > > Changes since v2: > > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net > > - Only keep the LSM hook move. > > > > 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 | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > This looks reasonable to me, and fixes a potential problem with > existing LSMs. Unless I hear any strong objections I'll plan to merge > this, and patch 2/2, into the LSM tree tomorrow. I didn't see these patches in -next, did I miss something? Landlock will use this hook really soon and it would make it much easier if these patches where upstream before. > > > diff --git a/fs/fcntl.c b/fs/fcntl.c > > index 300e5d9ad913..c28dc6c005f1 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) { > > @@ -98,19 +98,13 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, > > > > if (pid) { > > const struct cred *cred = current_cred(); > > + security_file_set_fowner(filp); > > filp->f_owner.uid = cred->uid; > > filp->f_owner.euid = cred->euid; > > } > > } > > 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 +140,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) > > -- > > 2.46.0 > > -- > paul-moore.com >
On Sun, Sep 8, 2024 at 2:11 PM Mickaël Salaün <mic@digikod.net> wrote: > > On Wed, Aug 21, 2024 at 12:32:17PM -0400, Paul Moore wrote: > > On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which > > > may not actually change the process that handles SIGIO (e.g. TUN, TTY, > > > dnotify), nor update the related UID/EUID. > > > > > > Moreover, because security_file_set_fowner() was called 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. > > > > > > This change makes sure the LSM states are always in sync with the VFS > > > state by moving the security_file_set_fowner() call close to the > > > UID/EUID updates and using the same f_owner.lock . > > > > > > 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> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > --- > > > > > > Changes since v2: > > > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net > > > - Only keep the LSM hook move. > > > > > > 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 | 14 ++++---------- > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > This looks reasonable to me, and fixes a potential problem with > > existing LSMs. Unless I hear any strong objections I'll plan to merge > > this, and patch 2/2, into the LSM tree tomorrow. > > I didn't see these patches in -next, did I miss something? > Landlock will use this hook really soon and it would make it much easier > if these patches where upstream before. Ah! My apologies, I'll do that right now and send another update once it's done. FWIW, I'm going to tag 1/2 for stable, but since we are at -rc7 presently I'll just plan to send it during the next merge window.
On Mon, Sep 9, 2024 at 12:03 PM Paul Moore <paul@paul-moore.com> wrote: > > On Sun, Sep 8, 2024 at 2:11 PM Mickaël Salaün <mic@digikod.net> wrote: > > > > On Wed, Aug 21, 2024 at 12:32:17PM -0400, Paul Moore wrote: > > > On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which > > > > may not actually change the process that handles SIGIO (e.g. TUN, TTY, > > > > dnotify), nor update the related UID/EUID. > > > > > > > > Moreover, because security_file_set_fowner() was called 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. > > > > > > > > This change makes sure the LSM states are always in sync with the VFS > > > > state by moving the security_file_set_fowner() call close to the > > > > UID/EUID updates and using the same f_owner.lock . > > > > > > > > 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> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > > --- > > > > > > > > Changes since v2: > > > > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net > > > > - Only keep the LSM hook move. > > > > > > > > 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 | 14 ++++---------- > > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > > > This looks reasonable to me, and fixes a potential problem with > > > existing LSMs. Unless I hear any strong objections I'll plan to merge > > > this, and patch 2/2, into the LSM tree tomorrow. > > > > I didn't see these patches in -next, did I miss something? > > Landlock will use this hook really soon and it would make it much easier > > if these patches where upstream before. > > Ah! My apologies, I'll do that right now and send another update once > it's done. FWIW, I'm going to tag 1/2 for stable, but since we are at > -rc7 presently I'll just plan to send it during the next merge window. Merged into lsm/dev, thanks for the nudge :)
On Mon, Sep 09, 2024 at 12:44:16PM -0400, Paul Moore wrote: > On Mon, Sep 9, 2024 at 12:03 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Sun, Sep 8, 2024 at 2:11 PM Mickaël Salaün <mic@digikod.net> wrote: > > > > > > On Wed, Aug 21, 2024 at 12:32:17PM -0400, Paul Moore wrote: > > > > On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which > > > > > may not actually change the process that handles SIGIO (e.g. TUN, TTY, > > > > > dnotify), nor update the related UID/EUID. > > > > > > > > > > Moreover, because security_file_set_fowner() was called 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. > > > > > > > > > > This change makes sure the LSM states are always in sync with the VFS > > > > > state by moving the security_file_set_fowner() call close to the > > > > > UID/EUID updates and using the same f_owner.lock . > > > > > > > > > > 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> > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > > > > > --- > > > > > > > > > > Changes since v2: > > > > > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net > > > > > - Only keep the LSM hook move. > > > > > > > > > > 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 | 14 ++++---------- > > > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > > > > > This looks reasonable to me, and fixes a potential problem with > > > > existing LSMs. Unless I hear any strong objections I'll plan to merge > > > > this, and patch 2/2, into the LSM tree tomorrow. > > > > > > I didn't see these patches in -next, did I miss something? > > > Landlock will use this hook really soon and it would make it much easier > > > if these patches where upstream before. > > > > Ah! My apologies, I'll do that right now and send another update once > > it's done. FWIW, I'm going to tag 1/2 for stable, but since we are at > > -rc7 presently I'll just plan to send it during the next merge window. > > Merged into lsm/dev, thanks for the nudge :) Thanks!
diff --git a/fs/fcntl.c b/fs/fcntl.c index 300e5d9ad913..c28dc6c005f1 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) { @@ -98,19 +98,13 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type, if (pid) { const struct cred *cred = current_cred(); + security_file_set_fowner(filp); filp->f_owner.uid = cred->uid; filp->f_owner.euid = cred->euid; } } 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 +140,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)
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 always called, ignoring the VFS logic which may not actually change the process that handles SIGIO (e.g. TUN, TTY, dnotify), nor update the related UID/EUID. Moreover, because security_file_set_fowner() was called 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. This change makes sure the LSM states are always in sync with the VFS state by moving the security_file_set_fowner() call close to the UID/EUID updates and using the same f_owner.lock . 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> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Mickaël Salaün <mic@digikod.net> --- Changes since v2: https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net - Only keep the LSM hook move. 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 | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)