Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <176640ae-3ff7-c3e9-218a-2952425336e7@linux.ibm.com>
Date:   Mon, 17 Apr 2023 08:45:40 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM after
 writes
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Amir Goldstein <amir73il@gmail.com>
Cc:     Paul Moore <paul@paul-moore.com>, zohar@linux.ibm.com,
        linux-integrity@vger.kernel.org, miklos@szeredi.hu,
        linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-unionfs@vger.kernel.org
References: <20230407-trasse-umgearbeitet-d580452b7a9b@brauner>
 <90a25725b4b3c96e84faefdb827b261901022606.camel@kernel.org>
 <e2455c0e-5a17-7fc1-95e3-5f2aca2eb409@linux.ibm.com>
 <94c2aadfb2fe7830d0289ffe6084581b99505a58.camel@kernel.org>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <94c2aadfb2fe7830d0289ffe6084581b99505a58.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/17/23 06:05, Jeff Layton wrote:
> On Sun, 2023-04-16 at 21:57 -0400, Stefan Berger wrote:
>>
>> On 4/7/23 09:29, Jeff Layton wrote:

>>>
>>> Note that there Stephen is correct that calling getattr is probably
>>> going to be less efficient here since we're going to end up calling
>>> generic_fillattr unnecessarily, but I still think it's the right thing
>>> to do.
>>
>> I was wondering whether to use the existing inode_eq_iversion() for all
>> other filesystems than overlayfs, nfs, and possibly other ones (which ones?)
>> where we would use the vfs_getattr_nosec() via a case on inode->i_sb->s_magic?
>> If so, would this function be generic enough to be a public function for libfs.c?
>>
>> I'll hopefully be able to test the proposed patch tomorrow.
>>
>>
> 
> No, you don't want to use inode_eq_iversion here because (as the comment
> over it says):

In the ima_check_last_writer() case the usage of inode_eq_iversion() was correct since
at this point no record of  its value was made and therefore no writer needed to change
the i_value again due to IMA:

		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)) {
			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
			iint->measured_pcrs = 0;
			if (update)
				ima_update_xattr(iint, file);
		}

The record of the value is only made when the actual measurement is done in
ima_collect_measurement()

Compared to this the usage of vfs_getattr_nosec() is expensive since it resets the flag.

         if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
                 stat->result_mask |= STATX_CHANGE_COOKIE;
                 stat->change_cookie = inode_query_iversion(inode);
         }

	idmap = mnt_idmap(path->mnt);
	if (inode->i_op->getattr)
		return inode->i_op->getattr(idmap, path, stat,
					    request_mask, query_flags);

Also, many filesystems will have their getattr now called as well.

I understand Christian's argument about the maintenance headache to a certain degree...

    Stefan

> 
>   * Note that we don't need to set the QUERIED flag in this case, as the value
>   * in the inode is not being recorded for later use.
> 
> The IMA code _does_ record the value for later use. Furthermore, it's
> not valid to use inode_eq_iversion on a non-IS_I_VERSION inode, so it's
> better to just use vfs_getattr_nosec which allows IMA to avoid all of
> those gory details.
> 
> Thanks,