Re: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM after writes

[Jeff Layton]

On Thu, 2023-04-06 at 17:01 +0200, Christian Brauner wrote:
> On Thu, Apr 06, 2023 at 10:36:41AM -0400, Paul Moore wrote:
> > On Thu, Apr 6, 2023 at 10:20 AM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
> > > On 4/6/23 10:05, Paul Moore wrote:
> > > > On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > > > > On Wed, Apr 05, 2023 at 01:14:49PM -0400, Stefan Berger wrote:
> > > > > > Overlayfs fails to notify IMA / EVM about file content modifications
> > > > > > and therefore IMA-appraised files may execute even though their file
> > > > > > signature does not validate against the changed hash of the file
> > > > > > anymore. To resolve this issue, add a call to integrity_notify_change()
> > > > > > to the ovl_release() function to notify the integrity subsystem about
> > > > > > file changes. The set flag triggers the re-evaluation of the file by
> > > > > > IMA / EVM once the file is accessed again.
> > > > > > 
> > > > > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > >   fs/overlayfs/file.c       |  4 ++++
> > > > > >   include/linux/integrity.h |  6 ++++++
> > > > > >   security/integrity/iint.c | 13 +++++++++++++
> > > > > >   3 files changed, 23 insertions(+)
> > > > > > 
> > > > > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > > > > > index 6011f955436b..19b8f4bcc18c 100644
> > > > > > --- a/fs/overlayfs/file.c
> > > > > > +++ b/fs/overlayfs/file.c
> > > > > > @@ -13,6 +13,7 @@
> > > > > >   #include <linux/security.h>
> > > > > >   #include <linux/mm.h>
> > > > > >   #include <linux/fs.h>
> > > > > > +#include <linux/integrity.h>
> > > > > >   #include "overlayfs.h"
> > > > > > 
> > > > > >   struct ovl_aio_req {
> > > > > > @@ -169,6 +170,9 @@ static int ovl_open(struct inode *inode, struct file *file)
> > > > > > 
> > > > > >   static int ovl_release(struct inode *inode, struct file *file)
> > > > > >   {
> > > > > > +     if (file->f_flags & O_ACCMODE)
> > > > > > +             integrity_notify_change(inode);
> > > > > > +
> > > > > >        fput(file->private_data);
> > > > > > 
> > > > > >        return 0;
> > > > > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> > > > > > index 2ea0f2f65ab6..cefdeccc1619 100644
> > > > > > --- a/include/linux/integrity.h
> > > > > > +++ b/include/linux/integrity.h
> > > > > > @@ -23,6 +23,7 @@ enum integrity_status {
> > > > > >   #ifdef CONFIG_INTEGRITY
> > > > > >   extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode);
> > > > > >   extern void integrity_inode_free(struct inode *inode);
> > > > > > +extern void integrity_notify_change(struct inode *inode);
> > > > > 
> > > > > I thought we concluded that ima is going to move into the security hook
> > > > > infrastructure so it seems this should be a proper LSM hook?
> > > > 
> > > > We are working towards migrating IMA/EVM to the LSM layer, but there
> > > > are a few things we need to fix/update/remove first; if anyone is
> > > > curious, you can join the LSM list as we've been discussing some of
> > > > these changes this week.  Bug fixes like this should probably remain
> > > > as IMA/EVM calls for the time being, with the understanding that they
> > > > will migrate over with the rest of IMA/EVM.
> > > > 
> > > > That said, we should give Mimi a chance to review this patch as it is
> > > > possible there is a different/better approach.  A bit of patience may
> > > > be required as I know Mimi is very busy at the moment.
> > > 
> > > There may be a better approach actually by increasing the inode's i_version,
> > > which then should trigger the appropriate path in ima_check_last_writer().
> > 
> > I'm not the VFS/inode expert here, but I thought the inode's i_version
> > field was only supposed to be bumped when the inode metadata changed,
> > not necessarily the file contents, right?
> > 

No. The i_version should change any time there is a "deliberate change"
to the file. That can be to the data or metadata, but it has to be in
response to some sort of deliberate, observable change -- something that
would cause an mtime or ctime change.

In practice, the i_version changes whenever the ctime changes, as
changing the mtime also changes the ctime.

> > That said, overlayfs is a bit different so maybe that's okay, but I
> > think we would need to hear from the VFS folks if this is acceptable.
> 
> Ccing Jeff for awareness since he did the i_version rework a short time ago.
> 
> The documentation in include/linux/iversion.h states:
> 
>  * [...] The i_version must
>  * appear larger to observers if there was an explicit change to the inode's
>  * data or metadata since it was last queried.
> 
> what I'm less sure in all of this is why this is called in ovl_release() and
> whether it's correct to increment the overlayfs inode's i_version.
>

Yeah, not sure what's special about doing this on close(). Seems like
someone could just hold the file open to prevent triggering the
remeasurement?

> The change is done to the inode of the copied up/modified file's inode in the
> upper layer. So the i_version should already be incremented when we call into
> the upper layer usually via vfs_*() methods.

Correct. As long as IMA is also measuring the upper inode then it seems
like you shouldn't need to do anything special here.

What sort of fs are you using for the upper layer?
-- 
Jeff Layton <jlayton@xxxxxxxxxx>