* [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
@ 2024-08-21 9:56 Mickaël Salaün
2024-08-21 9:56 ` [PATCH v3 2/2] security: Update file_set_fowner documentation Mickaël Salaün
2024-08-21 16:32 ` [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Paul Moore
0 siblings, 2 replies; 7+ messages in thread
From: Mickaël Salaün @ 2024-08-21 9:56 UTC (permalink / raw)
To: Christian Brauner, Paul Moore
Cc: Mickaël Salaün, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] security: Update file_set_fowner documentation
2024-08-21 9:56 [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Mickaël Salaün
@ 2024-08-21 9:56 ` Mickaël Salaün
2024-08-21 16:32 ` [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Paul Moore
1 sibling, 0 replies; 7+ messages in thread
From: Mickaël Salaün @ 2024-08-21 9:56 UTC (permalink / raw)
To: Christian Brauner, Paul Moore
Cc: Mickaël Salaün, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
Highlight that the file_set_fowner hook is now called with a lock held.
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>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v2:
- Split the doc update into a separate patch to ease backporting.
---
security/security.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/security.c b/security/security.c
index 8cee5b6c6e6d..dc2cd7354015 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2931,6 +2931,8 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
* Save owner security information (typically from current->security) in
* file->f_security for later use by the send_sigiotask hook.
*
+ * This hook is called with file->f_owner.lock held.
+ *
* Return: Returns 0 on success.
*/
void security_file_set_fowner(struct file *file)
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
2024-08-21 9:56 [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Mickaël Salaün
2024-08-21 9:56 ` [PATCH v3 2/2] security: Update file_set_fowner documentation Mickaël Salaün
@ 2024-08-21 16:32 ` Paul Moore
2024-09-08 18:11 ` Mickaël Salaün
1 sibling, 1 reply; 7+ messages in thread
From: Paul Moore @ 2024-08-21 16:32 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Christian Brauner, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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
--
paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
2024-08-21 16:32 ` [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Paul Moore
@ 2024-09-08 18:11 ` Mickaël Salaün
2024-09-09 16:03 ` Paul Moore
0 siblings, 1 reply; 7+ messages in thread
From: Mickaël Salaün @ 2024-09-08 18:11 UTC (permalink / raw)
To: Paul Moore
Cc: Christian Brauner, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
2024-09-08 18:11 ` Mickaël Salaün
@ 2024-09-09 16:03 ` Paul Moore
2024-09-09 16:44 ` Paul Moore
0 siblings, 1 reply; 7+ messages in thread
From: Paul Moore @ 2024-09-09 16:03 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Christian Brauner, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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.
--
paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
2024-09-09 16:03 ` Paul Moore
@ 2024-09-09 16:44 ` Paul Moore
2024-09-09 18:43 ` Mickaël Salaün
0 siblings, 1 reply; 7+ messages in thread
From: Paul Moore @ 2024-09-09 16:44 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Christian Brauner, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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 :)
--
paul-moore.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
2024-09-09 16:44 ` Paul Moore
@ 2024-09-09 18:43 ` Mickaël Salaün
0 siblings, 0 replies; 7+ messages in thread
From: Mickaël Salaün @ 2024-09-09 18:43 UTC (permalink / raw)
To: Paul Moore
Cc: Christian Brauner, linux-fsdevel, linux-kernel,
linux-security-module, selinux, Jan Kara, Tahera Fahimi,
Mateusz Guzik, Al Viro, Casey Schaufler, James Morris, Jann Horn,
Ondrej Mosnacek, Serge E. Hallyn, Stephen Smalley
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-09 18:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-21 9:56 [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Mickaël Salaün
2024-08-21 9:56 ` [PATCH v3 2/2] security: Update file_set_fowner documentation Mickaël Salaün
2024-08-21 16:32 ` [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies Paul Moore
2024-09-08 18:11 ` Mickaël Salaün
2024-09-09 16:03 ` Paul Moore
2024-09-09 16:44 ` Paul Moore
2024-09-09 18:43 ` Mickaël Salaün
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).