diff mbox series

[v9,12/25] security: Introduce file_post_open hook

Message ID 20240115181809.885385-13-roberto.sassu@huaweicloud.com
State New
Headers
Series security: Move IMA and EVM to the LSM infrastructure |

Commit Message

Roberto Sassu Jan. 15, 2024, 6:17 p.m. UTC 
  From: Roberto Sassu <roberto.sassu@huawei.com>

In preparation to move IMA and EVM to the LSM infrastructure, introduce the
file_post_open hook. Also, export security_file_post_open() for NFS.

Based on policy, IMA calculates the digest of the file content and
extends the TPM with the digest, verifies the file's integrity based on
the digest, and/or includes the file digest in the audit log.

LSMs could similarly take action depending on the file content and the
access mask requested with open().

The new hook returns a value and can cause the open to be aborted.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 fs/namei.c                    |  2 ++
 fs/nfsd/vfs.c                 |  6 ++++++
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  6 ++++++
 security/security.c           | 17 +++++++++++++++++
 5 files changed, 32 insertions(+)

Comments

Paul Moore Feb. 8, 2024, 3:18 a.m. UTC | #1 
On Jan 15, 2024 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> file_post_open hook. Also, export security_file_post_open() for NFS.
> 
> Based on policy, IMA calculates the digest of the file content and
> extends the TPM with the digest, verifies the file's integrity based on
> the digest, and/or includes the file digest in the audit log.
> 
> LSMs could similarly take action depending on the file content and the
> access mask requested with open().
> 
> The new hook returns a value and can cause the open to be aborted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  fs/namei.c                    |  2 ++
>  fs/nfsd/vfs.c                 |  6 ++++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/security.c           | 17 +++++++++++++++++
>  5 files changed, 32 insertions(+)

Acked-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com
Christian Brauner Feb. 9, 2024, 9:56 a.m. UTC | #2 
On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> file_post_open hook. Also, export security_file_post_open() for NFS.
> 
> Based on policy, IMA calculates the digest of the file content and
> extends the TPM with the digest, verifies the file's integrity based on
> the digest, and/or includes the file digest in the audit log.
> 
> LSMs could similarly take action depending on the file content and the
> access mask requested with open().
> 
> The new hook returns a value and can cause the open to be aborted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  fs/namei.c                    |  2 ++

Acked-by: Christian Brauner <brauner@kernel.org>
Christian Brauner Feb. 9, 2024, 9:59 a.m. UTC | #3 
On Fri, Feb 09, 2024 at 10:56:33AM +0100, Christian Brauner wrote:
> On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> > file_post_open hook. Also, export security_file_post_open() for NFS.
> > 
> > Based on policy, IMA calculates the digest of the file content and
> > extends the TPM with the digest, verifies the file's integrity based on
> > the digest, and/or includes the file digest in the audit log.
> > 
> > LSMs could similarly take action depending on the file content and the
> > access mask requested with open().
> > 
> > The new hook returns a value and can cause the open to be aborted.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  fs/namei.c                    |  2 ++
> 
> Acked-by: Christian Brauner <brauner@kernel.org>

Redacting that. I have a question.
Christian Brauner Feb. 9, 2024, 10:12 a.m. UTC | #4 
On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> file_post_open hook. Also, export security_file_post_open() for NFS.
> 
> Based on policy, IMA calculates the digest of the file content and
> extends the TPM with the digest, verifies the file's integrity based on
> the digest, and/or includes the file digest in the audit log.
> 
> LSMs could similarly take action depending on the file content and the
> access mask requested with open().
> 
> The new hook returns a value and can cause the open to be aborted.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  fs/namei.c                    |  2 ++
>  fs/nfsd/vfs.c                 |  6 ++++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/security.c           | 17 +++++++++++++++++
>  5 files changed, 32 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 71c13b2990b4..fb93d3e13df6 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3620,6 +3620,8 @@ static int do_open(struct nameidata *nd,
>  	error = may_open(idmap, &nd->path, acc_mode, open_flag);
>  	if (!error && !(file->f_mode & FMODE_OPENED))
>  		error = vfs_open(&nd->path, file);
> +	if (!error)
> +		error = security_file_post_open(file, op->acc_mode);

What does it do for O_CREAT? IOW, we managed to create that thing and we
managed to open that thing. Can security_file_post_open() and
ima_file_check() fail afterwards even for newly created files?
Roberto Sassu Feb. 9, 2024, 10:46 a.m. UTC | #5 
On Fri, 2024-02-09 at 11:12 +0100, Christian Brauner wrote:
> On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> > file_post_open hook. Also, export security_file_post_open() for NFS.
> > 
> > Based on policy, IMA calculates the digest of the file content and
> > extends the TPM with the digest, verifies the file's integrity based on
> > the digest, and/or includes the file digest in the audit log.
> > 
> > LSMs could similarly take action depending on the file content and the
> > access mask requested with open().
> > 
> > The new hook returns a value and can cause the open to be aborted.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  fs/namei.c                    |  2 ++
> >  fs/nfsd/vfs.c                 |  6 ++++++
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  6 ++++++
> >  security/security.c           | 17 +++++++++++++++++
> >  5 files changed, 32 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 71c13b2990b4..fb93d3e13df6 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3620,6 +3620,8 @@ static int do_open(struct nameidata *nd,
> >  	error = may_open(idmap, &nd->path, acc_mode, open_flag);
> >  	if (!error && !(file->f_mode & FMODE_OPENED))
> >  		error = vfs_open(&nd->path, file);
> > +	if (!error)
> > +		error = security_file_post_open(file, op->acc_mode);
> 
> What does it do for O_CREAT? IOW, we managed to create that thing and we
> managed to open that thing. Can security_file_post_open() and
> ima_file_check() fail afterwards even for newly created files?

$ strace touch test-file
..
openat(AT_FDCWD, "test-file", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0666) = -1 EPERM (Operation not permitted)

The open fails, but the file is there. I didn't see warnings/errors in
the kernel log.

Roberto
Christian Brauner Feb. 9, 2024, 11:34 a.m. UTC | #6 
On Fri, Feb 09, 2024 at 11:46:16AM +0100, Roberto Sassu wrote:
> On Fri, 2024-02-09 at 11:12 +0100, Christian Brauner wrote:
> > On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> > > file_post_open hook. Also, export security_file_post_open() for NFS.
> > > 
> > > Based on policy, IMA calculates the digest of the file content and
> > > extends the TPM with the digest, verifies the file's integrity based on
> > > the digest, and/or includes the file digest in the audit log.
> > > 
> > > LSMs could similarly take action depending on the file content and the
> > > access mask requested with open().
> > > 
> > > The new hook returns a value and can cause the open to be aborted.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > ---
> > >  fs/namei.c                    |  2 ++
> > >  fs/nfsd/vfs.c                 |  6 ++++++
> > >  include/linux/lsm_hook_defs.h |  1 +
> > >  include/linux/security.h      |  6 ++++++
> > >  security/security.c           | 17 +++++++++++++++++
> > >  5 files changed, 32 insertions(+)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 71c13b2990b4..fb93d3e13df6 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -3620,6 +3620,8 @@ static int do_open(struct nameidata *nd,
> > >  	error = may_open(idmap, &nd->path, acc_mode, open_flag);
> > >  	if (!error && !(file->f_mode & FMODE_OPENED))
> > >  		error = vfs_open(&nd->path, file);
> > > +	if (!error)
> > > +		error = security_file_post_open(file, op->acc_mode);
> > 
> > What does it do for O_CREAT? IOW, we managed to create that thing and we
> > managed to open that thing. Can security_file_post_open() and
> > ima_file_check() fail afterwards even for newly created files?
> 
> $ strace touch test-file
> ...
> openat(AT_FDCWD, "test-file", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0666) = -1 EPERM (Operation not permitted)

Ah, meh. I was hoping IMA just wouldn't care about this case.
Roberto Sassu Feb. 9, 2024, 12:02 p.m. UTC | #7 
On Fri, 2024-02-09 at 12:34 +0100, Christian Brauner wrote:
> On Fri, Feb 09, 2024 at 11:46:16AM +0100, Roberto Sassu wrote:
> > On Fri, 2024-02-09 at 11:12 +0100, Christian Brauner wrote:
> > > On Mon, Jan 15, 2024 at 07:17:56PM +0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> > > > file_post_open hook. Also, export security_file_post_open() for NFS.
> > > > 
> > > > Based on policy, IMA calculates the digest of the file content and
> > > > extends the TPM with the digest, verifies the file's integrity based on
> > > > the digest, and/or includes the file digest in the audit log.
> > > > 
> > > > LSMs could similarly take action depending on the file content and the
> > > > access mask requested with open().
> > > > 
> > > > The new hook returns a value and can cause the open to be aborted.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > > ---
> > > >  fs/namei.c                    |  2 ++
> > > >  fs/nfsd/vfs.c                 |  6 ++++++
> > > >  include/linux/lsm_hook_defs.h |  1 +
> > > >  include/linux/security.h      |  6 ++++++
> > > >  security/security.c           | 17 +++++++++++++++++
> > > >  5 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > index 71c13b2990b4..fb93d3e13df6 100644
> > > > --- a/fs/namei.c
> > > > +++ b/fs/namei.c
> > > > @@ -3620,6 +3620,8 @@ static int do_open(struct nameidata *nd,
> > > >  	error = may_open(idmap, &nd->path, acc_mode, open_flag);
> > > >  	if (!error && !(file->f_mode & FMODE_OPENED))
> > > >  		error = vfs_open(&nd->path, file);
> > > > +	if (!error)
> > > > +		error = security_file_post_open(file, op->acc_mode);
> > > 
> > > What does it do for O_CREAT? IOW, we managed to create that thing and we
> > > managed to open that thing. Can security_file_post_open() and
> > > ima_file_check() fail afterwards even for newly created files?
> > 
> > $ strace touch test-file
> > ...
> > openat(AT_FDCWD, "test-file", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0666) = -1 EPERM (Operation not permitted)
> 
> Ah, meh. I was hoping IMA just wouldn't care about this case.

Actually it doesn't. I added code to artifically create the situation
(to see what happens if a new LSM does that).

Roberto
Mimi Zohar Feb. 12, 2024, 9 p.m. UTC | #8 
Hi Roberto,


> diff --git a/security/security.c b/security/security.c
> index d9d2636104db..f3d92bffd02f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
>  	return fsnotify_perm(file, MAY_OPEN);  <===  Conflict

Replace with "return fsnotify_open_perm(file);"

>  }
>  

The patch set doesn't apply cleaning to 6.8-rcX without this change.  Unless
there are other issues, I can make the change.

thanks,

Mimi
Paul Moore Feb. 12, 2024, 9:16 p.m. UTC | #9 
On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Roberto,
>
>
> > diff --git a/security/security.c b/security/security.c
> > index d9d2636104db..f3d92bffd02f 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
>
> Replace with "return fsnotify_open_perm(file);"
>
> >  }
> >
>
> The patch set doesn't apply cleaning to 6.8-rcX without this change.  Unless
> there are other issues, I can make the change.

I take it this means you want to pull this via the IMA/EVM tree?
Roberto Sassu Feb. 13, 2024, 12:58 p.m. UTC | #10 
On Mon, 2024-02-12 at 16:16 -0500, Paul Moore wrote:
> On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > Hi Roberto,
> > 
> > 
> > > diff --git a/security/security.c b/security/security.c
> > > index d9d2636104db..f3d92bffd02f 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> > >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
> > 
> > Replace with "return fsnotify_open_perm(file);"
> > 
> > >  }
> > > 
> > 
> > The patch set doesn't apply cleaning to 6.8-rcX without this change.  Unless
> > there are other issues, I can make the change.
> 
> I take it this means you want to pull this via the IMA/EVM tree?

Not sure about that, but I have enough changes to do to make a v10.

Roberto
Paul Moore Feb. 13, 2024, 3:33 p.m. UTC | #11 
On Tue, Feb 13, 2024 at 7:59 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Mon, 2024-02-12 at 16:16 -0500, Paul Moore wrote:
> > On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > Hi Roberto,
> > >
> > >
> > > > diff --git a/security/security.c b/security/security.c
> > > > index d9d2636104db..f3d92bffd02f 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> > > >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
> > >
> > > Replace with "return fsnotify_open_perm(file);"
> > >
> > > >  }
> > > >
> > >
> > > The patch set doesn't apply cleaning to 6.8-rcX without this change.  Unless
> > > there are other issues, I can make the change.
> >
> > I take it this means you want to pull this via the IMA/EVM tree?
>
> Not sure about that, but I have enough changes to do to make a v10.

Sorry, I should have been more clear, the point I was trying to
resolve was who was going to take this patchset (eventually).  There
are other patches destined for the LSM tree that touch the LSM hooks
in a way which will cause conflicts with this patchset, and if
you/Mimi are going to take this via the IMA/EVM tree - which is fine
with me - I need to take that into account when merging things in the
LSM tree during this cycle.  It's not a big deal either way, it would
just be nice to get an answer on that within the next week.
Mimi Zohar Feb. 14, 2024, 8:07 p.m. UTC | #12 
On Tue, 2024-02-13 at 10:33 -0500, Paul Moore wrote:
> On Tue, Feb 13, 2024 at 7:59 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Mon, 2024-02-12 at 16:16 -0500, Paul Moore wrote:
> > > On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > Hi Roberto,
> > > > 
> > > > 
> > > > > diff --git a/security/security.c b/security/security.c
> > > > > index d9d2636104db..f3d92bffd02f 100644
> > > > > --- a/security/security.c
> > > > > +++ b/security/security.c
> > > > > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> > > > >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
> > > > 
> > > > Replace with "return fsnotify_open_perm(file);"
> > > > 
> > > > >  }
> > > > > 
> > > > 
> > > > The patch set doesn't apply cleaning to 6.8-rcX without this
> > > > change.  Unless
> > > > there are other issues, I can make the change.
> > > 
> > > I take it this means you want to pull this via the IMA/EVM tree?
> > 
> > Not sure about that, but I have enough changes to do to make a v10.

@Roberto:  please add my "Reviewed-by" to the remaining patches.

> 
> Sorry, I should have been more clear, the point I was trying to
> resolve was who was going to take this patchset (eventually).  There
> are other patches destined for the LSM tree that touch the LSM hooks
> in a way which will cause conflicts with this patchset, and if
> you/Mimi are going to take this via the IMA/EVM tree - which is fine
> with me - I need to take that into account when merging things in the
> LSM tree during this cycle.  It's not a big deal either way, it would
> just be nice to get an answer on that within the next week.

Similarly there are other changes for IMA and EVM.  If you're willing to create
a topic branch for just the v10 patch set that can be merged into your tree and
into my tree, I'm fine with your upstreaming v10. (I'll wait to send my pull
request after yours.)  Roberto will add my Ack's to the integrity, IMA, and EVM
related patches.  However if you're not willing to create a topic branch, I'll
upstream the v10 patch set.

thanks,

Mimi
Paul Moore Feb. 14, 2024, 9:21 p.m. UTC | #13 
On Wed, Feb 14, 2024 at 3:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Tue, 2024-02-13 at 10:33 -0500, Paul Moore wrote:
> > On Tue, Feb 13, 2024 at 7:59 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Mon, 2024-02-12 at 16:16 -0500, Paul Moore wrote:
> > > > On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibmcom> wrote:
> > > > > Hi Roberto,
> > > > >
> > > > >
> > > > > > diff --git a/security/security.c b/security/security.c
> > > > > > index d9d2636104db..f3d92bffd02f 100644
> > > > > > --- a/security/security.c
> > > > > > +++ b/security/security.c
> > > > > > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> > > > > >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
> > > > >
> > > > > Replace with "return fsnotify_open_perm(file);"
> > > > >
> > > > > >  }
> > > > > >
> > > > >
> > > > > The patch set doesn't apply cleaning to 6.8-rcX without this
> > > > > change.  Unless
> > > > > there are other issues, I can make the change.
> > > >
> > > > I take it this means you want to pull this via the IMA/EVM tree?
> > >
> > > Not sure about that, but I have enough changes to do to make a v10.
>
> @Roberto:  please add my "Reviewed-by" to the remaining patches.
>
> >
> > Sorry, I should have been more clear, the point I was trying to
> > resolve was who was going to take this patchset (eventually).  There
> > are other patches destined for the LSM tree that touch the LSM hooks
> > in a way which will cause conflicts with this patchset, and if
> > you/Mimi are going to take this via the IMA/EVM tree - which is fine
> > with me - I need to take that into account when merging things in the
> > LSM tree during this cycle.  It's not a big deal either way, it would
> > just be nice to get an answer on that within the next week.
>
> Similarly there are other changes for IMA and EVM.  If you're willing to create
> a topic branch for just the v10 patch set that can be merged into your tree and
> into my tree, I'm fine with your upstreaming v10. (I'll wait to send my pull
> request after yours.)  Roberto will add my Ack's to the integrity, IMA, and EVM
> related patches.  However if you're not willing to create a topic branch, I'll
> upstream the v10 patch set.

I'm not a big fan of sharing topic branches across different subsystem
trees, I'd much rather just agree that one tree or another takes the
patchset and the others plan accordingly.  Based on our previous
discussions I was under the impression that you wanted me to merge
this patchset into lsm/dev, but it looks like that is no longer the
case - which is okay by me.

Assuming Roberto gets a v10 out soon, do you expect to merge the v10
patchset and send it up during the upcoming merge window (for v6.9),
or are you expecting to wait until after the upcoming merge window
closes and target v6.10?  Once again, either is fine, I'm just trying
to coordinate this with other patches.
Mimi Zohar Feb. 15, 2024, 8:16 a.m. UTC | #14 
On Wed, 2024-02-14 at 16:21 -0500, Paul Moore wrote:
> On Wed, Feb 14, 2024 at 3:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Tue, 2024-02-13 at 10:33 -0500, Paul Moore wrote:
> > > On Tue, Feb 13, 2024 at 7:59 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > On Mon, 2024-02-12 at 16:16 -0500, Paul Moore wrote:
> > > > > On Mon, Feb 12, 2024 at 4:06 PM Mimi Zohar <zohar@linux.ibm.com>
> > > > > wrote:
> > > > > > Hi Roberto,
> > > > > > 
> > > > > > 
> > > > > > > diff --git a/security/security.c b/security/security.c
> > > > > > > index d9d2636104db..f3d92bffd02f 100644
> > > > > > > --- a/security/security.c
> > > > > > > +++ b/security/security.c
> > > > > > > @@ -2972,6 +2972,23 @@ int security_file_open(struct file *file)
> > > > > > >       return fsnotify_perm(file, MAY_OPEN);  <===  Conflict
> > > > > > 
> > > > > > Replace with "return fsnotify_open_perm(file);"
> > > > > > 
> > > > > > >  }
> > > > > > > 
> > > > > > 
> > > > > > The patch set doesn't apply cleaning to 6.8-rcX without this
> > > > > > change.  Unless
> > > > > > there are other issues, I can make the change.
> > > > > 
> > > > > I take it this means you want to pull this via the IMA/EVM tree?
> > > > 
> > > > Not sure about that, but I have enough changes to do to make a v10.
> > 
> > @Roberto:  please add my "Reviewed-by" to the remaining patches.
> > 
> > > Sorry, I should have been more clear, the point I was trying to
> > > resolve was who was going to take this patchset (eventually).  There
> > > are other patches destined for the LSM tree that touch the LSM hooks
> > > in a way which will cause conflicts with this patchset, and if
> > > you/Mimi are going to take this via the IMA/EVM tree - which is fine
> > > with me - I need to take that into account when merging things in the
> > > LSM tree during this cycle.  It's not a big deal either way, it would
> > > just be nice to get an answer on that within the next week.
> > 
> > Similarly there are other changes for IMA and EVM.  If you're willing to
> > create
> > a topic branch for just the v10 patch set that can be merged into your tree
> > and
> > into my tree, I'm fine with your upstreaming v10. (I'll wait to send my pull
> > request after yours.)  Roberto will add my Ack's to the integrity, IMA, and
> > EVM
> > related patches.  However if you're not willing to create a topic branch,
> > I'll
> > upstream the v10 patch set.
> 
> I'm not a big fan of sharing topic branches across different subsystem
> trees, I'd much rather just agree that one tree or another takes the
> patchset and the others plan accordingly.

Just curious why not?

> Based on our previous
> discussions I was under the impression that you wanted me to merge
> this patchset into lsm/dev, but it looks like that is no longer the
> case - which is okay by me.

Paul, I don't recall saying that.  Please go ahead and upstream it.  Roberto can
add my acks accordingly.

Mimi

> Assuming Roberto gets a v10 out soon, do you expect to merge the v10
> patchset and send it up during the upcoming merge window (for v6.9),
> or are you expecting to wait until after the upcoming merge window
> closes and target v6.10?  Once again, either is fine, I'm just trying
> to coordinate this with other patches.
Paul Moore Feb. 15, 2024, 3:02 p.m. UTC | #15 
On Thu, Feb 15, 2024 at 3:18 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Wed, 2024-02-14 at 16:21 -0500, Paul Moore wrote:
> > I'm not a big fan of sharing topic branches across different subsystem
> > trees, I'd much rather just agree that one tree or another takes the
> > patchset and the others plan accordingly.
>
> Just curious why not?

I don't like the idea of cross-tree dependencies, I realize the term
"dependency" isn't a great fit for a shared topic branch - no one
needs to feel the need to explain how pulls and merges work - but it's
the conceptual idea of there being a dependency across different trees
that bothers me.  I also tend to dislike the idea that a new feature
*absolutely* *must* *be* *in* *a* *certain* *release* to the point
that we need to subvert our normal processes to make it happen.

Further, I believe that shared topic branches also discourages
cooperation and collaboration.  With a topic branch, anyone who wants
to build on top of it simply merges the topic branch and off they go;
without a shared topic branch there needs to be a discussion about
which other patches are affected, which trees are involved, who is
going to carry the patches, when are they going up to Linus, etc.  As
someone who feels strongly that we need more collaboration across
kernel subsystems, I'm always going to pick the option that involves
developers talking with other developers outside their immediate
subsystem.

Hopefully that makes sense.

> > Based on our previous
> > discussions I was under the impression that you wanted me to merge
> > this patchset into lsm/dev, but it looks like that is no longer the
> > case - which is okay by me.
>
> Paul, I don't recall saying that.  Please go ahead and upstream it.  Roberto can
> add my acks accordingly.

I believe it was during an off-list chat when we were discussing an
earlier revision of the patchset, however, as I said earlier I'm not
bothered by who merges the patches, as long as they eventually end up
in Linus' tree I'm happy :)  I *really* want to stress that last bit,
if you and Roberto have stuff queued for the IMA/EVM tree that depends
on this patchset, please go ahead and merge it; you've got my ACKs on
the patches that need them, and I believe I've reviewed most of the
other patches that don't require my ACK.  While there are a some LSM
related patches that would sit on top of this patchset, there is
nothing that is so critical that it must go in now.

If I don't hear anything back from you, I'll go ahead and merge these
into lsm/dev later tonight (probably in about ~12 hours from this
email as I have some personal commitments early this evening) just so
we can get them into linux-next as soon as possible.
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 71c13b2990b4..fb93d3e13df6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3620,6 +3620,8 @@  static int do_open(struct nameidata *nd,
 	error = may_open(idmap, &nd->path, acc_mode, open_flag);
 	if (!error && !(file->f_mode & FMODE_OPENED))
 		error = vfs_open(&nd->path, file);
+	if (!error)
+		error = security_file_post_open(file, op->acc_mode);
 	if (!error)
 		error = ima_file_check(file, op->acc_mode);
 	if (!error && do_truncate)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fbbea7498f02..b0c3f07a8bba 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -877,6 +877,12 @@  __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 		goto out;
 	}
 
+	host_err = security_file_post_open(file, may_flags);
+	if (host_err) {
+		fput(file);
+		goto out;
+	}
+
 	host_err = ima_file_check(file, may_flags);
 	if (host_err) {
 		fput(file);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index fbca96523c18..c3fecc7dcb0b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -191,6 +191,7 @@  LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
 	 struct fown_struct *fown, int sig)
 LSM_HOOK(int, 0, file_receive, struct file *file)
 LSM_HOOK(int, 0, file_open, struct file *file)
+LSM_HOOK(int, 0, file_post_open, struct file *file, int mask)
 LSM_HOOK(int, 0, file_truncate, struct file *file)
 LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
 	 unsigned long clone_flags)
diff --git a/include/linux/security.h b/include/linux/security.h
index 84ae03690340..97f2212c13b6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -411,6 +411,7 @@  int security_file_send_sigiotask(struct task_struct *tsk,
 				 struct fown_struct *fown, int sig);
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file);
+int security_file_post_open(struct file *file, int mask);
 int security_file_truncate(struct file *file);
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
@@ -1074,6 +1075,11 @@  static inline int security_file_open(struct file *file)
 	return 0;
 }
 
+static inline int security_file_post_open(struct file *file, int mask)
+{
+	return 0;
+}
+
 static inline int security_file_truncate(struct file *file)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index d9d2636104db..f3d92bffd02f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2972,6 +2972,23 @@  int security_file_open(struct file *file)
 	return fsnotify_perm(file, MAY_OPEN);
 }
 
+/**
+ * security_file_post_open() - Evaluate a file after it has been opened
+ * @file: the file
+ * @mask: access mask
+ *
+ * Evaluate an opened file and the access mask requested with open(). The hook
+ * is useful for LSMs that require the file content to be available in order to
+ * make decisions.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_file_post_open(struct file *file, int mask)
+{
+	return call_int_hook(file_post_open, 0, file, mask);
+}
+EXPORT_SYMBOL_GPL(security_file_post_open);
+
 /**
  * security_file_truncate() - Check if truncating a file is allowed
  * @file: file