lkml.org 
[lkml]   [2024]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash
    From


    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.
    >

    \
     
     \ /
      Last update: 2024-05-27 14:43    [W:5.275 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site