Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger <stefanb@linux.ibm.com>]

On 2/1/24 09:11, Amir Goldstein wrote:
> On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 2/1/24 07:10, Amir Goldstein wrote:
>>> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/31/24 12:23, Amir Goldstein wrote:
>>>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 1/31/24 08:16, Amir Goldstein wrote:
>>>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 1/30/24 16:46, Stefan Berger wrote:
>>>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>>>>>>>>> are not take into account when d_backing_inode() is used when a file is
>>>>>>>>>> accessed on the overlay layer and this file has not yet been copied up.
>>>>>>>>>> This is because d_backing_inode() does not return the real inode of the
>>>>>>>>>> lower layer but instead returns the backing inode which holds old file
>>>>>>>>>> attributes. When the old file attributes are used for calculating the
>>>>>>>>>> metadata hash then the expected hash is calculated and the file then
>>>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>>>>>>>>> which returns the inode of the lower layer for as long as the file has
>>>>>>>>>> not been copied up and returns the upper layer's inode otherwise.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>>>>>>> ---
>>>>>>>>>>       security/integrity/evm/evm_crypto.c | 2 +-
>>>>>>>>>>       1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>>>>>>>>> --- a/security/integrity/evm/evm_crypto.c
>>>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>>>>>>>>                                    size_t req_xattr_value_len,
>>>>>>>>>>                                    uint8_t type, struct evm_digest *data)
>>>>>>>>>>       {
>>>>>>>>>> -     struct inode *inode = d_backing_inode(dentry);
>>>>>>>>>> +     struct inode *inode = d_real_inode(dentry);
>>>>>>>>>>           struct xattr_list *xattr;
>>>>>>>>>>           struct shash_desc *desc;
>>>>>>>>>>           size_t xattr_size = 0;
>>>>>>>>>
>>>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>>>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>>>>>>>>> not sure what the solution is.
>>>>>>>>
>>>>>>>> I think d_real_inode() does not work correctly for all its current users for
>>>>>>>> a metacopy file.
>>>>>>>>
>>>>>>>> I think the solution is to change d_real_inode() to return the data inode
>>>>>>>> and add another helper to get the metadata inode if needed.
>>>>>>>> I will post some patches for it.
>>>>>>>
>>>>>>> I thought that we may have to go through vfs_getattr() but even better
>>>>>>> if we don't because we don't have the file *file anywhere 'near'.
>>>>>>>
>>>>>>>>
>>>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
>>>>>>>> needs the lower data inode, the upper metadata inode or both.
>>>>>>>
>>>>>>> What it needs are data structures with mode bits, uid, and gid that stat
>>>>>>> in userspace would show.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
>>>>>> are always taken from the upper most inode which is what d_real_inode()
>>>>>> currently returns, so I do not understand what the problem is.
>>>>>>
>>>>>
>>>>> No, I was wrong. It is the other way around.
>>>>> d_real_inode() always returns the real data inode and you need the
>>>>> upper most real inode.
>>>>>
>>>>> You can try this:
>>>>>
>>>>>     -     struct inode *inode = d_backing_inode(dentry);
>>>>> +     struct inode *inode = d_inode(d_real(dentry, false));
>>>>>
>>>>> With the changes in:
>>>>>
>>>>> https://github.com/amir73il/linux/commits/overlayfs-devel/
>>>>>
>>>>> Not thoroughly tested...
>>>>
>>>> The change + 3 topmost patches cherry-picked is unfortunately are
>>>> crashing for me.
>>>>
>>>
>>> I will look into it.
>>> But anyway, the patch I suggested above is not enough exactly because
>>> of the reason I told you earlier.
>>>
>>> Mimi's fix ("ima: detect changes to the backing overlay file") detects
>>> a change in d_real_inode(file_dentry(file)) in order to invalidate the
>>> IMA cache.
>>>
>>> Your change also invalidates EVM cache on a change in
>>> d_real_inode(file_dentry(file)) and that makes sense.
>>>
>>> But on "meta copy up" for example on chmod(), an upper inode with no data
>>> is created (a metacopy) and all the attributes and xattr are copied
>>> from lower inode.
>>> The data remains in the lower inode.
>>>
>>> At this point , the IMA cache and the EVM cache refer to two different inodes
>>
>> You mean they refer to different inodes because IMA cares about file
>> content ("data remains in the lower inode:) and EVM cares about the
>> metadata ("an upper inode with no data is created")? If so, I agree
> 
> Correct.
> 
>> since the following line after copy-up with meatacopy enabled shows the
>> proper GID is in the backing inode not the one return from
>> d_real_inode(). If we knew that a meta copy has been done we could call
>> d_backing_inode() in this case for access to mode bits, uid, and gid.
>>
> 
> You should be able to use
> d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out.
> 
>> +       printk(KERN_INFO "real: GID: %d  backing: GID: %d\n",
>> +              from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid),
>> +              from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid));
>> +
>>
>>   > so you cannot share the same logic with IMA cache invalidation.
>>
>> I thought we we would have to share the same logic since IMA and EVM
>> would have to refer to the same inode also since IMA and EVM are
>> strongly connected. So the file_inode(file), which is typically used for
>> finding the iint, should be the same 'high level' inode for both EVM and
>> IMA I thought. A different inode could then be used for file data and
>> metadata.
>>
>>>
>>> My patches are meant to provide you with a helper e.g. d_real_meta_inode()
>>
>> The patch providing this isn't there yet in overlayfs-devel, right?
> 
> It's there just not spelled out with these helper names:
> 
> d_real_meta_inode(d) := d_inode(d_real(dentry, false))
> d_real_data_inode(d) := d_inode(d_real(dentry, true))
> d_real_inode(d) := d_real_data_inode(d)
> 
> I think this use case is pretty specific to EVM, so I don't think
> I will actually define these d_real_*_inode() helpers.
> 
>>
>>> that you could use to get the upper inode in the case of a metacopy, but
>>> IMA would still need to use the d_real_data_inode().
>>
>> That would be fine since we are only changing EVM code in this case.
> 
> Yes, using those overlayfs APIs requires understanding of what they mean
> and knowing how to test the affected use cases.
> This is not something very common for other subsystem developers.
> 
>>>
>>> Is that explanation clear? Is it clear why I said that the problem is more
>>> complicated?
>>
>> I think I understand it.
>>
> 
> I think I am also starting to understand the expectation from IMA/EVM
> over overlayfs ;)
> 
> Anyway, let me see if I can fix the problem in my WIP branch.

The good news is that with your two patches applied :

3b0ed3977dd2 (HEAD) fs: make file_dentry() a simple accessor
b5ccc40f3d50 fs: remove the inode argument to ->d_real() method

and your suggested change to this patch :

-       struct inode *inode = d_real_inode(dentry);
+       struct inode *inode = d_inode(d_real(dentry, false));;


The test cases are now passing with and without metacopy enabled. Yay!

Your 3rd patch causes crashes..

    Stefan


> 
> Thanks,
> Amir.