| Message ID | 20180122162452.8756-3-alban@kinvolk.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 22, 2018 at 05:24:52PM +0100, Alban Crequy wrote: > From: Alban Crequy <alban@kinvolk.io> > > This patch forces files to be re-measured, re-appraised and re-audited > on file systems with the feature flag FS_IMA_NO_CACHE. In that way, > cached integrity results won't be used. > > How to test this: > > The test I did was using a patched version of the memfs FUSE driver > [1][2] and two very simple "hello-world" programs [4] (prog1 prints > "hello world: 1" and prog2 prints "hello world: 2"). > > I copy prog1 and prog2 in the fuse-memfs mount point, execute them and > check the sha1 hash in > "/sys/kernel/security/ima/ascii_runtime_measurements". > > My patch on the memfs FUSE driver added a backdoor command to serve > prog1 when the kernel asks for prog2 or vice-versa. In this way, I can > exec prog1 and get it to print "hello world: 2" without ever replacing > the file via the VFS, so the kernel is not aware of the change. > > The test was done using the branch "alban/fuse-flag-ima-nocache-v3" [3]. > > Step by step test procedure: > > 1. Mount the memfs FUSE using [2]: > rm -f /tmp/memfs-switch* ; memfs -L DEBUG /mnt/memfs > > 2. Copy prog1 and prog2 using [4] > cp prog1 /mnt/memfs/prog1 > cp prog2 /mnt/memfs/prog2 > > 3. Lookup the files and let the FUSE driver to keep the handles open: > dd if=/mnt/memfs/prog1 bs=1 | (read -n 1 x ; sleep 3600 ) & > dd if=/mnt/memfs/prog2 bs=1 | (read -n 1 x ; sleep 3600 ) & > > 4. Check the 2 programs work correctly: > $ /mnt/memfs/prog1 > hello world: 1 > $ /mnt/memfs/prog2 > hello world: 2 > > 5. Check the measurements for prog1 and prog2: > $ sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > | grep /mnt/memfs/prog > 10 [...] ima-ng sha1:ac14c9268cd2[...] /mnt/memfs/prog1 > 10 [...] ima-ng sha1:799cb5d1e06d[...] /mnt/memfs/prog2 > > 6. Use the backdoor command in my patched memfs to redirect file > operations on file handle 3 to file handle 2: > rm -f /tmp/memfs-switch* ; touch /tmp/memfs-switch-3-2 > > 7. Check how the FUSE driver serves different content for the files: > $ /mnt/memfs/prog1 > hello world: 2 > $ /mnt/memfs/prog2 > hello world: 2 > > 8. Check the measurements: > sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > | grep /mnt/memfs/prog > > Without the patch, there are no new measurements, despite the FUSE > driver having served different executables. > > With the patch, I can see additional measurements for prog1 and prog2 > with the hashes reversed when the FUSE driver served the alternative > content. > > [1] https://github.com/bbengfort/memfs > [2] https://github.com/kinvolk/memfs/commits/alban/switch-files > [3] https://github.com/kinvolk/linux/commits/alban/fuse-flag-ima-nocache-v3 > [4] https://github.com/kinvolk/fuse-userns-patches/commit/cf1f5750cab0 > > Cc: linux-kernel@vger.kernel.org > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: James Morris <james.l.morris@oracle.com> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: Seth Forshee <seth.forshee@canonical.com> > Cc: Christoph Hellwig <hch@infradead.org> > Tested-by: Dongsu Park <dongsu@kinvolk.io> > Signed-off-by: Alban Crequy <alban@kinvolk.io> I like this approach as it doesn't require any IMA policy changes to be effective. I'm not sure about the flag name (in my mind it would describe the fs property and not be specifically giving instructions to IMA), but if others are happy with it I won't complain. Acked-by: Seth Forshee <seth.forshee@canonical.com> > --- > security/integrity/ima/ima_main.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 6d78cb26784d..8870a7bbe9b9 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -24,6 +24,7 @@ > #include <linux/slab.h> > #include <linux/xattr.h> > #include <linux/ima.h> > +#include <linux/fs.h> > > #include "ima.h" > > @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | > IMA_ACTION_FLAGS); > > - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) > - /* reset all flags if ima_inode_setxattr was called */ > + /* > + * Reset the measure, appraise and audit cached flags either if: > + * - ima_inode_setxattr was called, or > + * - based on filesystem feature flag > + * forcing the file to be re-evaluated. > + */ > + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { > iint->flags &= ~IMA_DONE_MASK; > + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { > + if (action & IMA_MEASURE) { > + iint->measured_pcrs = 0; > + iint->flags &= > + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); > + } > + if (action & IMA_APPRAISE) > + iint->flags &= > + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | > + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); > + if (action & IMA_AUDIT) > + iint->flags &= > + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); > + } > > /* Determine if already appraised/measured based on bitmask > * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, > -- > 2.13.6 > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Alban Crequy (alban.crequy@gmail.com): > From: Alban Crequy <alban@kinvolk.io> > > This patch forces files to be re-measured, re-appraised and re-audited > on file systems with the feature flag FS_IMA_NO_CACHE. In that way, > cached integrity results won't be used. > > How to test this: > > The test I did was using a patched version of the memfs FUSE driver > [1][2] and two very simple "hello-world" programs [4] (prog1 prints > "hello world: 1" and prog2 prints "hello world: 2"). > > I copy prog1 and prog2 in the fuse-memfs mount point, execute them and > check the sha1 hash in > "/sys/kernel/security/ima/ascii_runtime_measurements". > > My patch on the memfs FUSE driver added a backdoor command to serve > prog1 when the kernel asks for prog2 or vice-versa. In this way, I can > exec prog1 and get it to print "hello world: 2" without ever replacing > the file via the VFS, so the kernel is not aware of the change. > > The test was done using the branch "alban/fuse-flag-ima-nocache-v3" [3]. > > Step by step test procedure: > > 1. Mount the memfs FUSE using [2]: > rm -f /tmp/memfs-switch* ; memfs -L DEBUG /mnt/memfs > > 2. Copy prog1 and prog2 using [4] > cp prog1 /mnt/memfs/prog1 > cp prog2 /mnt/memfs/prog2 > > 3. Lookup the files and let the FUSE driver to keep the handles open: > dd if=/mnt/memfs/prog1 bs=1 | (read -n 1 x ; sleep 3600 ) & > dd if=/mnt/memfs/prog2 bs=1 | (read -n 1 x ; sleep 3600 ) & > > 4. Check the 2 programs work correctly: > $ /mnt/memfs/prog1 > hello world: 1 > $ /mnt/memfs/prog2 > hello world: 2 > > 5. Check the measurements for prog1 and prog2: > $ sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > | grep /mnt/memfs/prog > 10 [...] ima-ng sha1:ac14c9268cd2[...] /mnt/memfs/prog1 > 10 [...] ima-ng sha1:799cb5d1e06d[...] /mnt/memfs/prog2 > > 6. Use the backdoor command in my patched memfs to redirect file > operations on file handle 3 to file handle 2: > rm -f /tmp/memfs-switch* ; touch /tmp/memfs-switch-3-2 > > 7. Check how the FUSE driver serves different content for the files: > $ /mnt/memfs/prog1 > hello world: 2 > $ /mnt/memfs/prog2 > hello world: 2 > > 8. Check the measurements: > sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > | grep /mnt/memfs/prog > > Without the patch, there are no new measurements, despite the FUSE > driver having served different executables. > > With the patch, I can see additional measurements for prog1 and prog2 > with the hashes reversed when the FUSE driver served the alternative > content. > > [1] https://github.com/bbengfort/memfs > [2] https://github.com/kinvolk/memfs/commits/alban/switch-files > [3] https://github.com/kinvolk/linux/commits/alban/fuse-flag-ima-nocache-v3 > [4] https://github.com/kinvolk/fuse-userns-patches/commit/cf1f5750cab0 > > Cc: linux-kernel@vger.kernel.org > Cc: linux-integrity@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: Miklos Szeredi <miklos@szeredi.hu> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: James Morris <james.l.morris@oracle.com> > Cc: "Serge E. Hallyn" <serge@hallyn.com> Acked-by: Serge Hallyn <serge@hallyn.com> to both. > Cc: Seth Forshee <seth.forshee@canonical.com> > Cc: Christoph Hellwig <hch@infradead.org> > Tested-by: Dongsu Park <dongsu@kinvolk.io> > Signed-off-by: Alban Crequy <alban@kinvolk.io> > --- > security/integrity/ima/ima_main.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 6d78cb26784d..8870a7bbe9b9 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -24,6 +24,7 @@ > #include <linux/slab.h> > #include <linux/xattr.h> > #include <linux/ima.h> > +#include <linux/fs.h> > > #include "ima.h" > > @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | > IMA_ACTION_FLAGS); > > - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) > - /* reset all flags if ima_inode_setxattr was called */ > + /* > + * Reset the measure, appraise and audit cached flags either if: > + * - ima_inode_setxattr was called, or > + * - based on filesystem feature flag > + * forcing the file to be re-evaluated. > + */ > + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { > iint->flags &= ~IMA_DONE_MASK; > + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { > + if (action & IMA_MEASURE) { > + iint->measured_pcrs = 0; > + iint->flags &= > + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); > + } > + if (action & IMA_APPRAISE) > + iint->flags &= > + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | > + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); > + if (action & IMA_AUDIT) > + iint->flags &= > + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); > + } > > /* Determine if already appraised/measured based on bitmask > * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED, > -- > 2.13.6 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Seth Forshee (seth.forshee@canonical.com): > On Mon, Jan 22, 2018 at 05:24:52PM +0100, Alban Crequy wrote: > > From: Alban Crequy <alban@kinvolk.io> > > > > This patch forces files to be re-measured, re-appraised and re-audited > > on file systems with the feature flag FS_IMA_NO_CACHE. In that way, > > cached integrity results won't be used. > > > > How to test this: > > > > The test I did was using a patched version of the memfs FUSE driver > > [1][2] and two very simple "hello-world" programs [4] (prog1 prints > > "hello world: 1" and prog2 prints "hello world: 2"). > > > > I copy prog1 and prog2 in the fuse-memfs mount point, execute them and > > check the sha1 hash in > > "/sys/kernel/security/ima/ascii_runtime_measurements". > > > > My patch on the memfs FUSE driver added a backdoor command to serve > > prog1 when the kernel asks for prog2 or vice-versa. In this way, I can > > exec prog1 and get it to print "hello world: 2" without ever replacing > > the file via the VFS, so the kernel is not aware of the change. > > > > The test was done using the branch "alban/fuse-flag-ima-nocache-v3" [3]. > > > > Step by step test procedure: > > > > 1. Mount the memfs FUSE using [2]: > > rm -f /tmp/memfs-switch* ; memfs -L DEBUG /mnt/memfs > > > > 2. Copy prog1 and prog2 using [4] > > cp prog1 /mnt/memfs/prog1 > > cp prog2 /mnt/memfs/prog2 > > > > 3. Lookup the files and let the FUSE driver to keep the handles open: > > dd if=/mnt/memfs/prog1 bs=1 | (read -n 1 x ; sleep 3600 ) & > > dd if=/mnt/memfs/prog2 bs=1 | (read -n 1 x ; sleep 3600 ) & > > > > 4. Check the 2 programs work correctly: > > $ /mnt/memfs/prog1 > > hello world: 1 > > $ /mnt/memfs/prog2 > > hello world: 2 > > > > 5. Check the measurements for prog1 and prog2: > > $ sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > > | grep /mnt/memfs/prog > > 10 [...] ima-ng sha1:ac14c9268cd2[...] /mnt/memfs/prog1 > > 10 [...] ima-ng sha1:799cb5d1e06d[...] /mnt/memfs/prog2 > > > > 6. Use the backdoor command in my patched memfs to redirect file > > operations on file handle 3 to file handle 2: > > rm -f /tmp/memfs-switch* ; touch /tmp/memfs-switch-3-2 > > > > 7. Check how the FUSE driver serves different content for the files: > > $ /mnt/memfs/prog1 > > hello world: 2 > > $ /mnt/memfs/prog2 > > hello world: 2 > > > > 8. Check the measurements: > > sudo cat /sys/kernel/security/ima/ascii_runtime_measurements \ > > | grep /mnt/memfs/prog > > > > Without the patch, there are no new measurements, despite the FUSE > > driver having served different executables. > > > > With the patch, I can see additional measurements for prog1 and prog2 > > with the hashes reversed when the FUSE driver served the alternative > > content. > > > > [1] https://github.com/bbengfort/memfs > > [2] https://github.com/kinvolk/memfs/commits/alban/switch-files > > [3] https://github.com/kinvolk/linux/commits/alban/fuse-flag-ima-nocache-v3 > > [4] https://github.com/kinvolk/fuse-userns-patches/commit/cf1f5750cab0 > > > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-integrity@vger.kernel.org > > Cc: linux-security-module@vger.kernel.org > > Cc: linux-fsdevel@vger.kernel.org > > Cc: Miklos Szeredi <miklos@szeredi.hu> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > > Cc: James Morris <james.l.morris@oracle.com> > > Cc: "Serge E. Hallyn" <serge@hallyn.com> > > Cc: Seth Forshee <seth.forshee@canonical.com> > > Cc: Christoph Hellwig <hch@infradead.org> > > Tested-by: Dongsu Park <dongsu@kinvolk.io> > > Signed-off-by: Alban Crequy <alban@kinvolk.io> > > I like this approach as it doesn't require any IMA policy changes to be > effective. I'm not sure about the flag name (in my mind it would > describe the fs property and not be specifically giving instructions to > IMA), but if others are happy with it I won't complain. Purely internal name at least, so if we come up with a better name we can trivially change it. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > > IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | > > IMA_ACTION_FLAGS); > > > > - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) > > - /* reset all flags if ima_inode_setxattr was called */ > > + /* > > + * Reset the measure, appraise and audit cached flags either if: > > + * - ima_inode_setxattr was called, or > > + * - based on filesystem feature flag > > + * forcing the file to be re-evaluated. > > + */ > > + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { > > iint->flags &= ~IMA_DONE_MASK; > > + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { > > + if (action & IMA_MEASURE) { > > + iint->measured_pcrs = 0; > > + iint->flags &= > > + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); > > + } > > + if (action & IMA_APPRAISE) > > + iint->flags &= > > + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | > > + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); > > + if (action & IMA_AUDIT) > > + iint->flags &= > > + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); > > + } > > Alban, I don't know what I was thinking, but this can be simplified like for the IMA_CHANGE_XATTR case. Except in the IMA_CHANGE_XATTR case, "measured_pcrs" was already reset, whereas in this case "measured_pcrs" needs to be reset. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alban, On Thu, 2018-01-25 at 06:56 -0500, Mimi Zohar wrote: > > > @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > > > IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | > > > IMA_ACTION_FLAGS); > > > > > > - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) > > > - /* reset all flags if ima_inode_setxattr was called */ > > > + /* > > > + * Reset the measure, appraise and audit cached flags either if: > > > + * - ima_inode_setxattr was called, or > > > + * - based on filesystem feature flag > > > + * forcing the file to be re-evaluated. > > > + */ > > > + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { > > > iint->flags &= ~IMA_DONE_MASK; > > > + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { > > > + if (action & IMA_MEASURE) { > > > + iint->measured_pcrs = 0; > > > + iint->flags &= > > > + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); > > > + } > > > + if (action & IMA_APPRAISE) > > > + iint->flags &= > > > + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | > > > + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); > > > + if (action & IMA_AUDIT) > > > + iint->flags &= > > > + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); > > > + } > > > > > Alban, I don't know what I was thinking, but this can be simplified > like for the IMA_CHANGE_XATTR case. Except in the IMA_CHANGE_XATTR > case, "measured_pcrs" was already reset, whereas in this case > "measured_pcrs" needs to be reset. Did you get a chance to make the change and test it? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mimi, On Mon, Jan 29, 2018 at 5:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > Hi Alban, > > On Thu, 2018-01-25 at 06:56 -0500, Mimi Zohar wrote: >> > > @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, >> > > IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | >> > > IMA_ACTION_FLAGS); >> > > >> > > - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) >> > > - /* reset all flags if ima_inode_setxattr was called */ >> > > + /* >> > > + * Reset the measure, appraise and audit cached flags either if: >> > > + * - ima_inode_setxattr was called, or >> > > + * - based on filesystem feature flag >> > > + * forcing the file to be re-evaluated. >> > > + */ >> > > + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { >> > > iint->flags &= ~IMA_DONE_MASK; >> > > + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { >> > > + if (action & IMA_MEASURE) { >> > > + iint->measured_pcrs = 0; >> > > + iint->flags &= >> > > + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); >> > > + } >> > > + if (action & IMA_APPRAISE) >> > > + iint->flags &= >> > > + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | >> > > + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); >> > > + if (action & IMA_AUDIT) >> > > + iint->flags &= >> > > + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); >> > > + } >> > > >> >> Alban, I don't know what I was thinking, but this can be simplified >> like for the IMA_CHANGE_XATTR case. Except in the IMA_CHANGE_XATTR >> case, "measured_pcrs" was already reset, whereas in this case >> "measured_pcrs" needs to be reset. > > Did you get a chance to make the change and test it? Alban has been on holidays, so he will be back on Wednesday or so. So I'll try to understand what you meant in the last email. As IMA_DONE_MASK contains all other bitmasks, it's possible to optimize the code like this: if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { iint->flags &= ~IMA_DONE_MASK; } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { iint->flags &= ~IMA_DONE_MASK; if (action & IMA_MEASURE) iint->measured_pcrs = 0; } Is that what you want to see? Please let me know if it's not. Tomorrow I will try to test with a new patch. Thanks, Dongsu > Mimi > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Jan 29, 2018 at 6:40 PM, Dongsu Park <dongsu@kinvolk.io> wrote: > On Mon, Jan 29, 2018 at 5:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: >> On Thu, 2018-01-25 at 06:56 -0500, Mimi Zohar wrote: ... >> Did you get a chance to make the change and test it? > > Alban has been on holidays, so he will be back on Wednesday or so. > So I'll try to understand what you meant in the last email. > > As IMA_DONE_MASK contains all other bitmasks, it's possible to > optimize the code like this: > > if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { > iint->flags &= ~IMA_DONE_MASK; > } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { > iint->flags &= ~IMA_DONE_MASK; > if (action & IMA_MEASURE) > iint->measured_pcrs = 0; > } > > Is that what you want to see? Please let me know if it's not. > Tomorrow I will try to test with a new patch. Today I created a new patch, and tested it. It worked fine. So I've just sent a new patchset v4. Please see: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1598387.html Thanks, Dongsu > Thanks, > Dongsu > >> Mimi >> -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 6d78cb26784d..8870a7bbe9b9 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/xattr.h> #include <linux/ima.h> +#include <linux/fs.h> #include "ima.h" @@ -228,9 +229,28 @@ static int process_measurement(struct file *file, char *buf, loff_t size, IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK | IMA_ACTION_FLAGS); - if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) - /* reset all flags if ima_inode_setxattr was called */ + /* + * Reset the measure, appraise and audit cached flags either if: + * - ima_inode_setxattr was called, or + * - based on filesystem feature flag + * forcing the file to be re-evaluated. + */ + if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags)) { iint->flags &= ~IMA_DONE_MASK; + } else if (inode->i_sb->s_type->fs_flags & FS_IMA_NO_CACHE) { + if (action & IMA_MEASURE) { + iint->measured_pcrs = 0; + iint->flags &= + ~(IMA_COLLECTED | IMA_MEASURE | IMA_MEASURED); + } + if (action & IMA_APPRAISE) + iint->flags &= + ~(IMA_COLLECTED | IMA_APPRAISE | IMA_APPRAISED | + IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK); + if (action & IMA_AUDIT) + iint->flags &= + ~(IMA_COLLECTED | IMA_AUDIT | IMA_AUDITED); + } /* Determine if already appraised/measured based on bitmask * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,