Re: [PATCH v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies

[Mickaël Salaün]

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@xxxxxxxxxxxxxx> wrote:
> >
> > On Sun, Sep 8, 2024 at 2:11 PM Mickaël Salaün <mic@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:
> > > > >
> > > > > The fcntl's F_SETOWN command sets the process that handle SIGIO/SIGURG
> > > > > for the related file descriptor.  Before this change, the
> > > > > file_set_fowner LSM hook was 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@xxxxxxxxxxxxxxxxxx>
> > > > > Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > > > > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > > > > Cc: James Morris <jmorris@xxxxxxxxx>
> > > > > Cc: Jann Horn <jannh@xxxxxxxxxx>
> > > > > Cc: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > > > Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> > > > > Cc: Serge E. Hallyn <serge@xxxxxxxxxx>
> > > > > Cc: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Changes since v2:
> > > > > https://lore.kernel.org/r/20240812174421.1636724-1-mic@xxxxxxxxxxx
> > > > > - Only keep the LSM hook move.
> > > > >
> > > > > Changes since v1:
> > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@xxxxxxxxxxx
> > > > > - Add back the file_set_fowner hook (but without user) as
> > > > >   requested by Paul, but move it for consistency.
> > > > > ---
> > > > >  fs/fcntl.c | 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!