linux-kernel - Re: [RFC PATCH v3 2/2] ima: force re-appraisal on filesystems with FS_IMA_NO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Jan 2018 18:40:16 +0100
From:   Dongsu Park <dongsu@...volk.io>
To:     Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:     "Serge E. Hallyn" <serge@...lyn.com>,
        Alban Crequy <alban.crequy@...il.com>,
        Alban Crequy <alban@...volk.io>,
        Iago López Galeiras <iago@...volk.io>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, Miklos Szeredi <miklos@...redi.hu>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        dmitry.kasatkin@...il.com,
        James Morris <james.l.morris@...cle.com>,
        Seth Forshee <seth.forshee@...onical.com>, hch@...radead.org
Subject: Re: [RFC PATCH v3 2/2] ima: force re-appraisal on filesystems with FS_IMA_NO_CACHE

Hi Mimi,

On Mon, Jan 29, 2018 at 5:33 PM, Mimi Zohar <zohar@...ux.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
>