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

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

On Tue, 2022-11-29 at 15:31 +0100, Nicolas Bouchinet wrote:
> Hi Roberto,
> 
> On Tue, Nov 29, 2022 at 02:10:06PM +0100, Roberto Sassu wrote:
> > 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.
> > 
> I think a Reported-by tag should enougth then !

Perfect, will do.

Thanks for reporting the issue.

Roberto

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