Messages in this thread | | | Date | Wed, 31 Jan 2024 12:46:50 -0500 | Subject | Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash | From | Stefan Berger <> |
| |
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.
FYI - in case you are interested: My tests running this with UML are here:
repo: https://github.com/stefanberger/ima-namespaces-tests.git branch: overlayfs
There's a UML config in config/config.uml Compiler kernel with this series applied: make ARCH=um -j128 && yes | cp linux /usr/local/bin/linux
sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-1/test.sh sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-2/test.sh sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-3/test.sh
The 2nd and 3rd test case will fail at some point when metacopy is enabled, otherwise they will all pass.
> > Thanks, > Amir. >
| |