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
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.
