Re: [PATCH v5] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Tue, 2022-11-29 at 13:58 +0100, Nicolas Bouchinet wrote:
> Hi Mimi,
> 
> On Tue, Nov 29, 2022 at 06:28:09AM -0500, Mimi Zohar wrote:
> > On Fri, 2022-11-25 at 16:57 +0100, Nicolas Bouchinet wrote:
> > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > 
> > > Fixes a NULL pointer dereference occurring in the
> > > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > > triggered if a `inode_init_security` hook returns 0 without initializing
> > > the given `struct xattr` fields (which is the case of BPF) and if no
> > > other LSM overrides thoses fields after. This also leads to memory
> > > leaks.
> > > 
> > > The `call_int_hook_xattr` macro has been inlined into the
> > > `security_inode_init_security` hook in order to check hooks return
> > > values and skip ones who doesn't init `xattrs`.
> > > 
> > > Modify `evm_init_hmac` function to init the EVM hmac using every
> > > entry of the given xattr array.
> > > 
> > > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > > compiled in, which gives room for SMACK, SELinux, Apparmor, BPF and
> > > IMA/EVM security attributes.
> > > 
> > > Changes the default return value of the `inode_init_security` hook
> > > definition to `-EOPNOTSUPP`.
> > > 
> > > Changes the hook documentation to match the behavior of the LSMs using
> > > it (only xattr->value is initialised with kmalloc and thus is the only
> > > one that should be kfreed by the caller).
> > > 
> > > Cc: roberto.sassu@huaweicloud.com
> > > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > What  is the relationship between this patch and Roberto's patch set? 
> > Roberto, if there is an overlap, then at minimum there should be a
> > Reported-by tag indicating that your patch set addresses a bug reported
> > by Nicolas.
> 
> This patch fixes the EVM NULL pointer dereference I have reported, and additionally
> improves the stackability of this LSM hook. This latter improvement was originally
> addressed by Roberto's patchset, and thus I see no problem for my fix to be merged
> within his patchset.

+       if (!num_filled_xattrs)
                goto out;
 
-       evm_xattr = lsm_xattr + 1;
-       ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
+       ret = evm_inode_init_security(inode, new_xattrs,
+                                     new_xattrs + num_filled_xattrs);

This part of patch 4 should be enough to fix the issue, until EVM is
outside the LSM infrastructure.

It prevents EVM from being called if there are no xattrs filled (the
panic occurred due to xattr->name being NULL).

Then, this part of patch 6:

+       for (xattr = xattrs; xattr->value != NULL; xattr++) {
+               if (evm_protected_xattr(xattr->name))
+                       evm_protected_xattrs = true;
+       }
+
+       /* EVM xattr not needed. */
+       if (!evm_protected_xattrs)
+               return -EOPNOTSUPP;

should be sufficient for when EVM is managed by the LSM infrastructure.

security_check_compact_filled_xattrs() ensures that if xattr->value is
not NULL, xattr->name is not NULL too.

Roberto

> > -- 
> > thanks,
> > 
> > Mimi
> > 
> 
> Thanks for your time,
> 
> Nicolas Bouchinet