* [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry
@ 2019-01-16 17:39 Goldwyn Rodrigues
2019-01-17 6:57 ` Amir Goldstein
0 siblings, 1 reply; 4+ messages in thread
From: Goldwyn Rodrigues @ 2019-01-16 17:39 UTC (permalink / raw)
To: linux-unionfs; +Cc: zohar, iforster, linux-integrity
Since copy_up() happens when you are modifying a file on overlay,
it is still a new file for the underlying filesystem. Mark it
in IMA for re-evaluating as a new file.
Putting ima calls within overlayfs may not be the best method, but this is
the only one which I thought would work.
Here is a test case:
mount /dev/vdb /lower
mount /dev/vdc /upper
echo "Original contents" > /lower/existingfile.txt
mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
echo "New contents" > /mnt/existingfile.txt
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
fs/overlayfs/copy_up.c | 8 ++++++++
security/integrity/ima/ima_main.c | 1 +
2 files changed, 9 insertions(+)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9e62dcf06fc4..f3f7f65ce4d3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -21,6 +21,7 @@
#include <linux/fdtable.h>
#include <linux/ratelimit.h>
#include <linux/exportfs.h>
+#include <linux/ima.h>
#include "overlayfs.h"
#define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -102,6 +103,11 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
goto retry;
}
+ if (!strcmp(name, XATTR_NAME_IMA)) {
+ ima_post_path_mknod(new);
+ continue;
+ }
+
error = security_inode_copy_up_xattr(name);
if (error < 0 && error != -EOPNOTSUPP)
break;
@@ -485,6 +491,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
err = ovl_set_size(temp, &c->stat);
if (!err)
err = ovl_set_attr(temp, &c->stat);
+ if (!err)
+ ima_post_path_mknod(c->dentry);
inode_unlock(temp->d_inode);
return err;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dbd4c8decde0..2229ea2a0ba6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -449,6 +449,7 @@ void ima_post_path_mknod(struct dentry *dentry)
/* needed for re-opening empty files */
iint->flags |= IMA_NEW_FILE;
}
+EXPORT_SYMBOL_GPL(ima_post_path_mknod);
/**
* ima_read_file - pre-measure/appraise hook decision based on policy
--
2.16.4
--
Goldwyn
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry
2019-01-16 17:39 [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry Goldwyn Rodrigues
@ 2019-01-17 6:57 ` Amir Goldstein
2019-01-17 15:02 ` Goldwyn Rodrigues
0 siblings, 1 reply; 4+ messages in thread
From: Amir Goldstein @ 2019-01-17 6:57 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: overlayfs, zohar, iforster, linux-integrity
On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> Since copy_up() happens when you are modifying a file on overlay,
> it is still a new file for the underlying filesystem. Mark it
> in IMA for re-evaluating as a new file.
>
> Putting ima calls within overlayfs may not be the best method, but this is
> the only one which I thought would work.
>
Doesn't look right.
Overlayfs creates the new inode with vfs_tmpfile() and I think that is
where you should plug the IMA hook.
> Here is a test case:
> mount /dev/vdb /lower
> mount /dev/vdc /upper
> echo "Original contents" > /lower/existingfile.txt
> mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> echo "New contents" > /mnt/existingfile.txt
>
I bet you can reproduce that same issue without overlayfs
by creating an O_TMPFILE from userspace.
The ima_file_check() hook in do_last() does not cover the O_TMPFILE
case.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry
2019-01-17 6:57 ` Amir Goldstein
@ 2019-01-17 15:02 ` Goldwyn Rodrigues
2019-01-17 15:44 ` Amir Goldstein
0 siblings, 1 reply; 4+ messages in thread
From: Goldwyn Rodrigues @ 2019-01-17 15:02 UTC (permalink / raw)
To: Amir Goldstein; +Cc: overlayfs, zohar, iforster, linux-integrity
On 8:57 17/01, Amir Goldstein wrote:
> On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > Since copy_up() happens when you are modifying a file on overlay,
> > it is still a new file for the underlying filesystem. Mark it
> > in IMA for re-evaluating as a new file.
> >
> > Putting ima calls within overlayfs may not be the best method, but this is
> > the only one which I thought would work.
> >
>
> Doesn't look right.
> Overlayfs creates the new inode with vfs_tmpfile() and I think that is
> where you should plug the IMA hook.
>
> > Here is a test case:
> > mount /dev/vdb /lower
> > mount /dev/vdc /upper
> > echo "Original contents" > /lower/existingfile.txt
> > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> > echo "New contents" > /mnt/existingfile.txt
> >
>
> I bet you can reproduce that same issue without overlayfs
> by creating an O_TMPFILE from userspace.
>
> The ima_file_check() hook in do_last() does not cover the O_TMPFILE
> case.
>
The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809
which I have in my tree.
The current patch is on top of that.
--
Goldwyn
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry
2019-01-17 15:02 ` Goldwyn Rodrigues
@ 2019-01-17 15:44 ` Amir Goldstein
0 siblings, 0 replies; 4+ messages in thread
From: Amir Goldstein @ 2019-01-17 15:44 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: overlayfs, zohar, iforster, linux-integrity
On Thu, Jan 17, 2019 at 5:02 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> On 8:57 17/01, Amir Goldstein wrote:
> > On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > >
> > > Since copy_up() happens when you are modifying a file on overlay,
> > > it is still a new file for the underlying filesystem. Mark it
> > > in IMA for re-evaluating as a new file.
> > >
> > > Putting ima calls within overlayfs may not be the best method, but this is
> > > the only one which I thought would work.
> > >
> >
> > Doesn't look right.
> > Overlayfs creates the new inode with vfs_tmpfile() and I think that is
> > where you should plug the IMA hook.
> >
> > > Here is a test case:
> > > mount /dev/vdb /lower
> > > mount /dev/vdc /upper
> > > echo "Original contents" > /lower/existingfile.txt
> > > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> > > echo "New contents" > /mnt/existingfile.txt
> > >
> >
> > I bet you can reproduce that same issue without overlayfs
> > by creating an O_TMPFILE from userspace.
> >
> > The ima_file_check() hook in do_last() does not cover the O_TMPFILE
> > case.
> >
>
> The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809
> which I have in my tree.
>
The proposed hook ima_post_create_tmpfile() inside do_tmpfile()
takes a file argument, uses only file_inode() and sets IMA_NEW_FILE.
Now because that hook does not get called from vfs_tmpfile()
you want to add more ima hook inside overlayfs code after calling
vfs_tmpfile().
If you move the IMA hook inside vfs_tmpfile() and pass the dentry
or inode, you will get the same result and you won't need to change
overlayfs code.
Is there a problem with that proposal?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-17 15:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 17:39 [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry Goldwyn Rodrigues
2019-01-17 6:57 ` Amir Goldstein
2019-01-17 15:02 ` Goldwyn Rodrigues
2019-01-17 15:44 ` Amir Goldstein
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.