| Message ID | 20230405171449.4064321-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | overlayfs: Trigger file re-evaluation by IMA / EVM after writes | expand |
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@linux.ibm.com> > --- > 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?
On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> > > --- > > 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.
On 4/6/23 10:05, Paul Moore wrote: > On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> >>> --- >>> 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().
On Thu, Apr 6, 2023 at 10:20 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > On 4/6/23 10:05, Paul Moore wrote: > > On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> > >>> --- > >>> 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? 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.
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@linux.ibm.com> wrote: > > On 4/6/23 10:05, Paul Moore wrote: > > > On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> > > >>> --- > > >>> 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? > > 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. 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.
On 4/6/23 10:36, Paul Moore wrote: > On Thu, Apr 6, 2023 at 10:20 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> On 4/6/23 10:05, Paul Moore wrote: >>> On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> >>>>> --- >>>>> 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? > > 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. > Exactly. In ima_check_last_writer() I want to trigger the code path with iint->flags &= ... if (atomic_read(&inode->i_writecount) == 1) { update = test_and_clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); if (!IS_I_VERSION(inode) || !inode_eq_iversion(inode, iint->version) || (iint->flags & IMA_NEW_FILE)) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); iint->measured_pcrs = 0; if (update) ima_update_xattr(iint, file); } } This patch here resolves it for my use case and triggers the expected code paths when ima_file_free() -> ima_check_last_writer() is called because then the i_version is seen as having been modified. diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 6011f955436b..1dfe5e7bfe1c 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/iversion.h> #include "overlayfs.h" struct ovl_aio_req { @@ -408,6 +409,8 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) if (ret != -EIOCBQUEUED) ovl_aio_cleanup_handler(aio_req); } + if (ret > 0) + inode_maybe_inc_iversion(inode, false); out: revert_creds(old_cred); out_fdput: I have been testing this in a OpenBMC/Yocto environment where overlayfs is used as root filesystem with the lower filesystem being a squashfs. Regards, Stefan
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@linux.ibm.com> wrote: > > > On 4/6/23 10:05, Paul Moore wrote: > > > > On Thu, Apr 6, 2023 at 6:26 AM Christian Brauner <brauner@kernel.org> 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@linux.ibm.com> > > > > > > --- > > > > > > 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?
On 4/6/23 14:46, Jeff Layton wrote: > 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: > > Correct. As long as IMA is also measuring the upper inode then it seems > like you shouldn't need to do anything special here. Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > > What sort of fs are you using for the upper layer? jffs2: /dev/mtdblock4 on /run/initramfs/ro type squashfs (ro,relatime,errors=continue) /dev/mtdblock5 on /run/initramfs/rw type jffs2 (rw,relatime) cow on / type overlay (rw,relatime,lowerdir=run/initramfs/ro,upperdir=run/initramfs/rw/cow,workdir=run/initramfs/rw/work) Regards, Stefan
On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: > > On 4/6/23 14:46, Jeff Layton wrote: > > 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: > > > > > Correct. As long as IMA is also measuring the upper inode then it seems > > like you shouldn't need to do anything special here. > > Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > It looks like remeasurement is usually done in ima_check_last_writer. That gets called from __fput which is called when we're releasing the last reference to the struct file. You've hooked into the ->release op, which gets called whenever filp_close is called, which happens when we're disassociating the file from the file descriptor table. So...I don't get it. Is ima_file_free not getting called on your file for some reason when you go to close it? It seems like that should be handling this. In any case, I think this could use a bit more root-cause analysis. > > > > What sort of fs are you using for the upper layer? > > jffs2: > > /dev/mtdblock4 on /run/initramfs/ro type squashfs (ro,relatime,errors=continue) > /dev/mtdblock5 on /run/initramfs/rw type jffs2 (rw,relatime) > cow on / type overlay (rw,relatime,lowerdir=run/initramfs/ro,upperdir=run/initramfs/rw/cow,workdir=run/initramfs/rw/work) > jffs2 does not have a proper i_version counter, I'm afraid. But, IMA should handle that OK (by assuming that it always needs to remeasure when there is no i_version counter).
On 4/6/23 15:37, Jeff Layton wrote: > On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: >> >> On 4/6/23 14:46, Jeff Layton wrote: >>> 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: >> >>> >>> Correct. As long as IMA is also measuring the upper inode then it seems >>> like you shouldn't need to do anything special here. >> >> Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. >> > > > It looks like remeasurement is usually done in ima_check_last_writer. > That gets called from __fput which is called when we're releasing the > last reference to the struct file. > > You've hooked into the ->release op, which gets called whenever > filp_close is called, which happens when we're disassociating the file > from the file descriptor table. > > So...I don't get it. Is ima_file_free not getting called on your file > for some reason when you go to close it? It seems like that should be > handling this. I would ditch the original proposal in favor of this 2-line patch shown here: https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 The new proposed i_version increase occurs on the inode that IMA sees later on for the file that's being executed and on which it must do a re-evaluation. Upon file changes ima_inode_free() seems to see two ima_file_free() calls, one for what seems to be the upper layer (used for vfs_* functions below) and once for the lower one. The important thing is that IMA will see the lower one when the file gets executed later on and this is the one that I instrumented now to have its i_version increased, which in turn triggers the re-evaluation of the file post modification. static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) [...] struct fd real; [...] ret = ovl_real_fdget(file, &real); if (ret) goto out_unlock; [...] if (is_sync_kiocb(iocb)) { file_start_write(real.file); --> ret = vfs_iter_write(real.file, iter, &iocb->ki_pos, ovl_iocb_to_rwf(ifl)); file_end_write(real.file); /* Update size */ ovl_copyattr(inode); } else { struct ovl_aio_req *aio_req; ret = -ENOMEM; aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL); if (!aio_req) goto out; file_start_write(real.file); /* Pacify lockdep, same trick as done in aio_write() */ __sb_writers_release(file_inode(real.file)->i_sb, SB_FREEZE_WRITE); aio_req->fd = real; real.flags = 0; aio_req->orig_iocb = iocb; kiocb_clone(&aio_req->iocb, iocb, real.file); aio_req->iocb.ki_flags = ifl; aio_req->iocb.ki_complete = ovl_aio_rw_complete; refcount_set(&aio_req->ref, 2); --> ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter); ovl_aio_put(aio_req); if (ret != -EIOCBQUEUED) ovl_aio_cleanup_handler(aio_req); } if (ret > 0) <--- this get it to work inode_maybe_inc_iversion(inode, false); <--- since this inode is known to IMA out: revert_creds(old_cred); out_fdput: fdput(real); out_unlock: inode_unlock(inode); Stefan > > In any case, I think this could use a bit more root-cause analysis.
On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: > > On 4/6/23 15:37, Jeff Layton wrote: > > On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: > > > > > > On 4/6/23 14:46, Jeff Layton wrote: > > > > 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: > > > > > > > > > > > Correct. As long as IMA is also measuring the upper inode then it seems > > > > like you shouldn't need to do anything special here. > > > > > > Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > > > > > > > > > It looks like remeasurement is usually done in ima_check_last_writer. > > That gets called from __fput which is called when we're releasing the > > last reference to the struct file. > > > > You've hooked into the ->release op, which gets called whenever > > filp_close is called, which happens when we're disassociating the file > > from the file descriptor table. > > > > So...I don't get it. Is ima_file_free not getting called on your file > > for some reason when you go to close it? It seems like that should be > > handling this. > > I would ditch the original proposal in favor of this 2-line patch shown here: > > https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 > > Ok, I think I get it. IMA is trying to use the i_version from the overlayfs inode. I suspect that the real problem here is that IMA is just doing a bare inode_query_iversion. Really, we ought to make IMA call vfs_getattr_nosec (or something like it) to query the getattr routine in the upper layer. Then overlayfs could just propagate the results from the upper layer in its response. That sort of design may also eventually help IMA work properly with more exotic filesystems, like NFS or Ceph. > The new proposed i_version increase occurs on the inode that IMA sees later on for > the file that's being executed and on which it must do a re-evaluation. > > Upon file changes ima_inode_free() seems to see two ima_file_free() calls, > one for what seems to be the upper layer (used for vfs_* functions below) > and once for the lower one. > The important thing is that IMA will see the lower one when the file gets > executed later on and this is the one that I instrumented now to have its > i_version increased, which in turn triggers the re-evaluation of the file post > modification. > > static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) > [...] > struct fd real; > [...] > ret = ovl_real_fdget(file, &real); > if (ret) > goto out_unlock; > > [...] > if (is_sync_kiocb(iocb)) { > file_start_write(real.file); > --> ret = vfs_iter_write(real.file, iter, &iocb->ki_pos, > ovl_iocb_to_rwf(ifl)); > file_end_write(real.file); > /* Update size */ > ovl_copyattr(inode); > } else { > struct ovl_aio_req *aio_req; > > ret = -ENOMEM; > aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL); > if (!aio_req) > goto out; > > file_start_write(real.file); > /* Pacify lockdep, same trick as done in aio_write() */ > __sb_writers_release(file_inode(real.file)->i_sb, > SB_FREEZE_WRITE); > aio_req->fd = real; > real.flags = 0; > aio_req->orig_iocb = iocb; > kiocb_clone(&aio_req->iocb, iocb, real.file); > aio_req->iocb.ki_flags = ifl; > aio_req->iocb.ki_complete = ovl_aio_rw_complete; > refcount_set(&aio_req->ref, 2); > --> ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter); > ovl_aio_put(aio_req); > if (ret != -EIOCBQUEUED) > ovl_aio_cleanup_handler(aio_req); > } > if (ret > 0) <--- this get it to work > inode_maybe_inc_iversion(inode, false); <--- since this inode is known to IMA > out: > revert_creds(old_cred); > out_fdput: > fdput(real); > > out_unlock: > inode_unlock(inode); > > > > > Stefan > > > > > In any case, I think this could use a bit more root-cause analysis. >
On 4/6/23 17:24, Jeff Layton wrote: > On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: >> >> On 4/6/23 15:37, Jeff Layton wrote: >>> On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: >>>> >>>> On 4/6/23 14:46, Jeff Layton wrote: >>>>> 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: >>>> >>>>> >>>>> Correct. As long as IMA is also measuring the upper inode then it seems >>>>> like you shouldn't need to do anything special here. >>>> >>>> Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. >>>> >>> >>> >>> It looks like remeasurement is usually done in ima_check_last_writer. >>> That gets called from __fput which is called when we're releasing the >>> last reference to the struct file. >>> >>> You've hooked into the ->release op, which gets called whenever >>> filp_close is called, which happens when we're disassociating the file >>> from the file descriptor table. >>> >>> So...I don't get it. Is ima_file_free not getting called on your file >>> for some reason when you go to close it? It seems like that should be >>> handling this. >> >> I would ditch the original proposal in favor of this 2-line patch shown here: >> >> https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 >> >> > > Ok, I think I get it. IMA is trying to use the i_version from the > overlayfs inode. > > I suspect that the real problem here is that IMA is just doing a bare > inode_query_iversion. Really, we ought to make IMA call > vfs_getattr_nosec (or something like it) to query the getattr routine in > the upper layer. Then overlayfs could just propagate the results from > the upper layer in its response. You mean compare known stat against current ? It seems more expensive to stat the file rather than using the simple i_version-has-changed indicator. > > That sort of design may also eventually help IMA work properly with more > exotic filesystems, like NFS or Ceph. And these don't support i_version at all? Stefan
On Thu, 2023-04-06 at 17:24 -0400, Jeff Layton wrote: > On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: > > > > On 4/6/23 15:37, Jeff Layton wrote: > > > On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: > > > > > > > > On 4/6/23 14:46, Jeff Layton wrote: > > > > > 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: > > > > > > > > > > > > > > Correct. As long as IMA is also measuring the upper inode then it seems > > > > > like you shouldn't need to do anything special here. > > > > > > > > Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > > > > > > > > > > > > > It looks like remeasurement is usually done in ima_check_last_writer. > > > That gets called from __fput which is called when we're releasing the > > > last reference to the struct file. > > > > > > You've hooked into the ->release op, which gets called whenever > > > filp_close is called, which happens when we're disassociating the file > > > from the file descriptor table. > > > > > > So...I don't get it. Is ima_file_free not getting called on your file > > > for some reason when you go to close it? It seems like that should be > > > handling this. > > > > I would ditch the original proposal in favor of this 2-line patch shown here: > > > > https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 > > > > > > Ok, I think I get it. IMA is trying to use the i_version from the > overlayfs inode. > > I suspect that the real problem here is that IMA is just doing a bare > inode_query_iversion. Really, we ought to make IMA call > vfs_getattr_nosec (or something like it) to query the getattr routine in > the upper layer. Then overlayfs could just propagate the results from > the upper layer in its response. > > That sort of design may also eventually help IMA work properly with more > exotic filesystems, like NFS or Ceph. > > > Maybe something like this? It builds for me but I haven't tested it. It looks like overlayfs already should report the upper layer's i_version in getattr, though I haven't tested that either: -----------------------8<--------------------------- [PATCH] IMA: use vfs_getattr_nosec to get the i_version IMA currently accesses the i_version out of the inode directly when it does a measurement. This is fine for most simple filesystems, but can be problematic with more complex setups (e.g. overlayfs). Make IMA instead call vfs_getattr_nosec to get this info. This allows the filesystem to determine whether and how to report the i_version, and should allow IMA to work properly with a broader class of filesystems in the future. Reported-by: Stefan Berger <stefanb@linux.ibm.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- security/integrity/ima/ima_api.c | 9 ++++++--- security/integrity/ima/ima_main.c | 12 ++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index d3662f4acadc..c45902e72044 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -13,7 +13,6 @@ #include <linux/fs.h> #include <linux/xattr.h> #include <linux/evm.h> -#include <linux/iversion.h> #include <linux/fsverity.h> #include "ima.h" @@ -246,10 +245,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, struct inode *inode = file_inode(file); const char *filename = file->f_path.dentry->d_name.name; struct ima_max_digest_data hash; + struct kstat stat; int result = 0; int length; void *tmpbuf; - u64 i_version; + u64 i_version = 0; /* * Always collect the modsig, because IMA might have already collected @@ -268,7 +268,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, * to an initial measurement/appraisal/audit, but was modified to * assume the file changed. */ - i_version = inode_query_iversion(inode); + result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE, + AT_STATX_SYNC_AS_STAT); + if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) + i_version = stat.change_cookie; hash.hdr.algo = algo; hash.hdr.length = hash_digest_size[algo]; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index d66a0a36415e..365db0e43d7c 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -24,7 +24,6 @@ #include <linux/slab.h> #include <linux/xattr.h> #include <linux/ima.h> -#include <linux/iversion.h> #include <linux/fs.h> #include "ima.h" @@ -164,11 +163,16 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, mutex_lock(&iint->mutex); if (atomic_read(&inode->i_writecount) == 1) { + struct kstat stat; + update = test_and_clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags); - if (!IS_I_VERSION(inode) || - !inode_eq_iversion(inode, iint->version) || - (iint->flags & IMA_NEW_FILE)) { + if ((iint->flags & IMA_NEW_FILE) || + vfs_getattr_nosec(&file->f_path, &stat, + STATX_CHANGE_COOKIE, + AT_STATX_SYNC_AS_STAT) || + !(stat.result_mask & STATX_CHANGE_COOKIE) || + stat.change_cookie != iint->version) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); iint->measured_pcrs = 0; if (update)
On Thu, 2023-04-06 at 17:58 -0400, Stefan Berger wrote: > > On 4/6/23 17:24, Jeff Layton wrote: > > On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: > > > > > > On 4/6/23 15:37, Jeff Layton wrote: > > > > On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: > > > > > > > > > > On 4/6/23 14:46, Jeff Layton wrote: > > > > > > 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: > > > > > > > > > > > > > > > > > Correct. As long as IMA is also measuring the upper inode then it seems > > > > > > like you shouldn't need to do anything special here. > > > > > > > > > > Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > > > > > > > > > > > > > > > > > It looks like remeasurement is usually done in ima_check_last_writer. > > > > That gets called from __fput which is called when we're releasing the > > > > last reference to the struct file. > > > > > > > > You've hooked into the ->release op, which gets called whenever > > > > filp_close is called, which happens when we're disassociating the file > > > > from the file descriptor table. > > > > > > > > So...I don't get it. Is ima_file_free not getting called on your file > > > > for some reason when you go to close it? It seems like that should be > > > > handling this. > > > > > > I would ditch the original proposal in favor of this 2-line patch shown here: > > > > > > https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 > > > > > > > > > > Ok, I think I get it. IMA is trying to use the i_version from the > > overlayfs inode. > > > > I suspect that the real problem here is that IMA is just doing a bare > > inode_query_iversion. Really, we ought to make IMA call > > vfs_getattr_nosec (or something like it) to query the getattr routine in > > the upper layer. Then overlayfs could just propagate the results from > > the upper layer in its response. > > You mean compare known stat against current ? It seems more expensive to stat the file > rather than using the simple i_version-has-changed indicator. > getattr is fairly cheap on a local filesystem. It's more expensive with something networked, but that's the price of correctness. > > That sort of design may also eventually help IMA work properly with more > > exotic filesystems, like NFS or Ceph. > > And these don't support i_version at all? They absolutely do. Their change attributes are mediated by the server, so they can't use the kernel's mechanism for IS_I_VERSION inodes. They can report that field in their ->getattr routines however.
On 4/6/23 18:04, Jeff Layton wrote: > On Thu, 2023-04-06 at 17:24 -0400, Jeff Layton wrote: >> On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: >>> >>> On 4/6/23 15:37, Jeff Layton wrote: >>>> On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: >>>>> >>>>> On 4/6/23 14:46, Jeff Layton wrote: >>>>>> 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: >>>>> >>>>>> >>>>>> Correct. As long as IMA is also measuring the upper inode then it seems >>>>>> like you shouldn't need to do anything special here. >>>>> >>>>> Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. >>>>> >>>> >>>> >>>> It looks like remeasurement is usually done in ima_check_last_writer. >>>> That gets called from __fput which is called when we're releasing the >>>> last reference to the struct file. >>>> >>>> You've hooked into the ->release op, which gets called whenever >>>> filp_close is called, which happens when we're disassociating the file >>>> from the file descriptor table. >>>> >>>> So...I don't get it. Is ima_file_free not getting called on your file >>>> for some reason when you go to close it? It seems like that should be >>>> handling this. >>> >>> I would ditch the original proposal in favor of this 2-line patch shown here: >>> >>> https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 >>> >>> >> >> Ok, I think I get it. IMA is trying to use the i_version from the >> overlayfs inode. >> >> I suspect that the real problem here is that IMA is just doing a bare >> inode_query_iversion. Really, we ought to make IMA call >> vfs_getattr_nosec (or something like it) to query the getattr routine in >> the upper layer. Then overlayfs could just propagate the results from >> the upper layer in its response. >> >> That sort of design may also eventually help IMA work properly with more >> exotic filesystems, like NFS or Ceph. >> >> >> > > Maybe something like this? It builds for me but I haven't tested it. It > looks like overlayfs already should report the upper layer's i_version > in getattr, though I haven't tested that either: Thank you! I will give it a try once I am back. Stefan
On Thu, Apr 6, 2023 at 11:23 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 4/6/23 15:37, Jeff Layton wrote: > > On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: > >> > >> On 4/6/23 14:46, Jeff Layton wrote: > >>> 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: > >> > >>> > >>> Correct. As long as IMA is also measuring the upper inode then it seems > >>> like you shouldn't need to do anything special here. > >> > >> Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. > >> > > > > > > It looks like remeasurement is usually done in ima_check_last_writer. > > That gets called from __fput which is called when we're releasing the > > last reference to the struct file. > > > > You've hooked into the ->release op, which gets called whenever > > filp_close is called, which happens when we're disassociating the file > > from the file descriptor table. > > > > So...I don't get it. Is ima_file_free not getting called on your file > > for some reason when you go to close it? It seems like that should be > > handling this. > > I would ditch the original proposal in favor of this 2-line patch shown here: > > https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 > > > The new proposed i_version increase occurs on the inode that IMA sees later on for > the file that's being executed and on which it must do a re-evaluation. > > Upon file changes ima_inode_free() seems to see two ima_file_free() calls, > one for what seems to be the upper layer (used for vfs_* functions below) > and once for the lower one. > The important thing is that IMA will see the lower one when the file gets > executed later on and this is the one that I instrumented now to have its > i_version increased, which in turn triggers the re-evaluation of the file post > modification. > > static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) > [...] > struct fd real; > [...] > ret = ovl_real_fdget(file, &real); > if (ret) > goto out_unlock; > > [...] > if (is_sync_kiocb(iocb)) { > file_start_write(real.file); > --> ret = vfs_iter_write(real.file, iter, &iocb->ki_pos, > ovl_iocb_to_rwf(ifl)); > file_end_write(real.file); > /* Update size */ > ovl_copyattr(inode); > } else { > struct ovl_aio_req *aio_req; > > ret = -ENOMEM; > aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL); > if (!aio_req) > goto out; > > file_start_write(real.file); > /* Pacify lockdep, same trick as done in aio_write() */ > __sb_writers_release(file_inode(real.file)->i_sb, > SB_FREEZE_WRITE); > aio_req->fd = real; > real.flags = 0; > aio_req->orig_iocb = iocb; > kiocb_clone(&aio_req->iocb, iocb, real.file); > aio_req->iocb.ki_flags = ifl; > aio_req->iocb.ki_complete = ovl_aio_rw_complete; > refcount_set(&aio_req->ref, 2); > --> ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter); > ovl_aio_put(aio_req); > if (ret != -EIOCBQUEUED) > ovl_aio_cleanup_handler(aio_req); > } > if (ret > 0) <--- this get it to work > inode_maybe_inc_iversion(inode, false); <--- since this inode is known to IMA If the aio is queued, then I think increasing i_version here may be premature. Note that in this code flow, the ovl ctime is updated in ovl_aio_cleanup_handler() => ovl_copyattr() after file_end_write(), similar to the is_sync_kiocb() code patch. It probably makes most sense to include i_version in ovl_copyattr(). Note that this could cause ovl i_version to go backwards on copy up (i.e. after first open for write) when switching from the lower inode i_version to the upper inode i_version. Jeff's proposal to use vfs_getattr_nosec() in IMA code is fine too. It will result in the same i_version jump. IMO it wouldn't hurt to have a valid i_version value in the ovl inode as well. If the ovl inode values would not matter, we would not have needed ovl_copyattr() at all, but it's not good to keep vfs in the dark... Thanks, Amir.
On 4/6/23 18:04, Jeff Layton wrote: > On Thu, 2023-04-06 at 17:24 -0400, Jeff Layton wrote: >> On Thu, 2023-04-06 at 16:22 -0400, Stefan Berger wrote: >>> >>> On 4/6/23 15:37, Jeff Layton wrote: >>>> On Thu, 2023-04-06 at 15:11 -0400, Stefan Berger wrote: >>>>> >>>>> On 4/6/23 14:46, Jeff Layton wrote: >>>>>> 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: >>>>> >>>>>> >>>>>> Correct. As long as IMA is also measuring the upper inode then it seems >>>>>> like you shouldn't need to do anything special here. >>>>> >>>>> Unfortunately IMA does not notice the changes. With the patch provided in the other email IMA works as expected. >>>>> >>>> >>>> >>>> It looks like remeasurement is usually done in ima_check_last_writer. >>>> That gets called from __fput which is called when we're releasing the >>>> last reference to the struct file. >>>> >>>> You've hooked into the ->release op, which gets called whenever >>>> filp_close is called, which happens when we're disassociating the file >>>> from the file descriptor table. >>>> >>>> So...I don't get it. Is ima_file_free not getting called on your file >>>> for some reason when you go to close it? It seems like that should be >>>> handling this. >>> >>> I would ditch the original proposal in favor of this 2-line patch shown here: >>> >>> https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232 >>> >>> >> >> Ok, I think I get it. IMA is trying to use the i_version from the >> overlayfs inode. >> >> I suspect that the real problem here is that IMA is just doing a bare >> inode_query_iversion. Really, we ought to make IMA call >> vfs_getattr_nosec (or something like it) to query the getattr routine in >> the upper layer. Then overlayfs could just propagate the results from >> the upper layer in its response. >> >> That sort of design may also eventually help IMA work properly with more >> exotic filesystems, like NFS or Ceph. >> >> >> > > Maybe something like this? It builds for me but I haven't tested it. It > looks like overlayfs already should report the upper layer's i_version > in getattr, though I haven't tested that either: > > -----------------------8<--------------------------- > > [PATCH] IMA: use vfs_getattr_nosec to get the i_version > > IMA currently accesses the i_version out of the inode directly when it > does a measurement. This is fine for most simple filesystems, but can be > problematic with more complex setups (e.g. overlayfs). > > Make IMA instead call vfs_getattr_nosec to get this info. This allows > the filesystem to determine whether and how to report the i_version, and > should allow IMA to work properly with a broader class of filesystems in > the future. > > Reported-by: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > security/integrity/ima/ima_api.c | 9 ++++++--- > security/integrity/ima/ima_main.c | 12 ++++++++---- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index d3662f4acadc..c45902e72044 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -13,7 +13,6 @@ > #include <linux/fs.h> > #include <linux/xattr.h> > #include <linux/evm.h> > -#include <linux/iversion.h> > #include <linux/fsverity.h> > > #include "ima.h" > @@ -246,10 +245,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > struct inode *inode = file_inode(file); > const char *filename = file->f_path.dentry->d_name.name; > struct ima_max_digest_data hash; > + struct kstat stat; > int result = 0; > int length; > void *tmpbuf; > - u64 i_version; > + u64 i_version = 0; > > /* > * Always collect the modsig, because IMA might have already collected > @@ -268,7 +268,10 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > * to an initial measurement/appraisal/audit, but was modified to > * assume the file changed. > */ > - i_version = inode_query_iversion(inode); > + result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE, > + AT_STATX_SYNC_AS_STAT); > + if (!result && (stat.result_mask & STATX_CHANGE_COOKIE)) > + i_version = stat.change_cookie; > hash.hdr.algo = algo; > hash.hdr.length = hash_digest_size[algo]; > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index d66a0a36415e..365db0e43d7c 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -24,7 +24,6 @@ > #include <linux/slab.h> > #include <linux/xattr.h> > #include <linux/ima.h> > -#include <linux/iversion.h> > #include <linux/fs.h> > > #include "ima.h" > @@ -164,11 +163,16 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, > > mutex_lock(&iint->mutex); > if (atomic_read(&inode->i_writecount) == 1) { > + struct kstat stat; > + > update = test_and_clear_bit(IMA_UPDATE_XATTR, > &iint->atomic_flags); > - if (!IS_I_VERSION(inode) || > - !inode_eq_iversion(inode, iint->version) || > - (iint->flags & IMA_NEW_FILE)) { > + if ((iint->flags & IMA_NEW_FILE) || > + vfs_getattr_nosec(&file->f_path, &stat, > + STATX_CHANGE_COOKIE, > + AT_STATX_SYNC_AS_STAT) || > + !(stat.result_mask & STATX_CHANGE_COOKIE) || > + stat.change_cookie != iint->version) { > iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); > iint->measured_pcrs = 0; > if (update) I tested this in the OpenBMC setup with overlayfs acting as rootfs. It works now as expected. Tested-by: Stefan Berger <stefanb@linux.ibm.com>
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); extern void __init integrity_load_keys(void); #else @@ -37,6 +38,11 @@ static inline void integrity_inode_free(struct inode *inode) return; } +static inline void integrity_notify_change(struct inode *inode) +{ + return; +} + static inline void integrity_load_keys(void) { } diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 8638976f7990..70d2d716f3ae 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -85,6 +85,19 @@ static void iint_free(struct integrity_iint_cache *iint) kmem_cache_free(iint_cache, iint); } +void integrity_notify_change(struct inode *inode) +{ + struct integrity_iint_cache *iint; + + if (!IS_IMA(inode)) + return; + + iint = integrity_iint_find(inode); + if (iint) + set_bit(IMA_CHANGE_XATTR, &iint->atomic_flags); +} +EXPORT_SYMBOL_GPL(integrity_notify_change); + /** * integrity_inode_get - find or allocate an iint associated with an inode * @inode: pointer to the inode
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@linux.ibm.com> --- fs/overlayfs/file.c | 4 ++++ include/linux/integrity.h | 6 ++++++ security/integrity/iint.c | 13 +++++++++++++ 3 files changed, 23 insertions(+)