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@xxxxxxxxxxxxx/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@xxxxxxxxxxxxx>
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
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)
