Received-SPF: pass (google.com: domain of linux-kernel+bounces-48085-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240130214620.3155380-1-stefanb@linux.ibm.com>
 <20240130214620.3155380-5-stefanb@linux.ibm.com> <38230b4c-54ae-45ed-a6fb-34e63501e5b1@linux.ibm.com>
 <CAOQ4uxiYARZBSgzb4_W-RKvB1XLSF3GUBqeLw2kH+eVeZ_8ARQ@mail.gmail.com>
 <c018b014-9ba8-4395-86dc-b61346ab20a8@linux.ibm.com> <CAOQ4uxi6Te8izWpXROthknRaXrVA9jho5nbc+mkuQDrcTLY44Q@mail.gmail.com>
 <CAOQ4uxigdNeE+2nfr4VxS9piQf5hez=ryT0a-jzW+tW0BT-zuw@mail.gmail.com> <492ea12a-d79d-47da-9bbe-a7f33051bd3f@linux.ibm.com>
In-Reply-To: <492ea12a-d79d-47da-9bbe-a7f33051bd3f@linux.ibm.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 1 Feb 2024 14:10:58 +0200
Message-ID: <CAOQ4uxgiO1RbsmqOu4F4Foy-MBPecnEXO7BvgDGz-Lzb1Eysog@mail.gmail.com>
Subject: Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate
 metadata hash
To: Stefan Berger <stefanb@linux.ibm.com>
Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, 
	linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org, 
	paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, zohar@linux.ibm.com, 
	roberto.sassu@huawei.com, miklos@szeredi.hu
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 7:46=E2=80=AFPM Stefan Berger <stefanb@linux.ibm.co=
m> wrote:
>
>
>
> On 1/31/24 12:23, Amir Goldstein wrote:
> > On Wed, Jan 31, 2024 at 5:54=E2=80=AFPM Amir Goldstein <amir73il@gmail.=
com> wrote:
> >>
> >> On Wed, Jan 31, 2024 at 4:40=E2=80=AFPM Stefan Berger <stefanb@linux.i=
bm.com> wrote:
> >>>
> >>>
> >>>
> >>> On 1/31/24 08:16, Amir Goldstein wrote:
> >>>> On Wed, Jan 31, 2024 at 4:11=E2=80=AFAM 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 l=
ayer
> >>>>>> are not take into account when d_backing_inode() is used when a fi=
le is
> >>>>>> accessed on the overlay layer and this file has not yet been copie=
d up.
> >>>>>> This is because d_backing_inode() does not return the real inode o=
f 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 th=
en
> >>>>>> mistakenly passes signature verification. Therefore, use d_real_in=
ode()
> >>>>>> 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/integr=
ity/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 =3D d_backing_inode(dentry);
> >>>>>> +     struct inode *inode =3D d_real_inode(dentry);
> >>>>>>         struct xattr_list *xattr;
> >>>>>>         struct shash_desc *desc;
> >>>>>>         size_t xattr_size =3D 0;
> >>>>>
> >>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY b=
ut
> >>>>> when setting CONFIG_OVERLAY_FS_METACOPY=3Dy 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 u=
sers 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 bette=
r
> >>> 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 s=
tat
> >>> in userspace would show.
> >>>
> >>>
> >>
> >> With or without metacopy enabled, an overlay inode st_uid st_gid st_mo=
de
> >> 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 =3D d_backing_inode(dentry);
> > +     struct inode *inode =3D 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 inod=
es
so you cannot share the same logic with IMA cache invalidation.

My patches are meant to provide you with a helper e.g. d_real_meta_inode()
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().

Is that explanation clear? Is it clear why I said that the problem is more
complicated?

Thanks,
Amir.