Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ef1ff552e16eadf5bc8d6219d463b35511103c38.camel@linux.ibm.com>
Subject: Re: [PATCH v9 3/4] evm: Align evm_inode_init_security() definition
 with LSM infrastructure
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huaweicloud.com>,
        Paul Moore <paul@paul-moore.com>
Cc:     dmitry.kasatkin@gmail.com, jmorris@namei.org, serge@hallyn.com,
        stephen.smalley.work@gmail.com, eparis@parisplace.org,
        casey@schaufler-ca.com, reiserfs-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
        bpf@vger.kernel.org, kpsingh@kernel.org, keescook@chromium.org,
        nicolas.bouchinet@clip-os.org,
        Roberto Sassu <roberto.sassu@huawei.com>
Date:   Mon, 03 Apr 2023 06:37:48 -0400
In-Reply-To: <ca6d4080b0f234b5321d965fb1350bfcd291646e.camel@huaweicloud.com>
References: <20230329130415.2312521-1-roberto.sassu@huaweicloud.com>
         <20230329130415.2312521-4-roberto.sassu@huaweicloud.com>
         <CAHC9VhSDVv30ce2652kridRU7iaQQ19tiGubWpyP0mi7pf+JJw@mail.gmail.com>
         <bfe74fff24a5a7a8059acc00c29ac957bf0b7880.camel@huaweicloud.com>
         <ca6d4080b0f234b5321d965fb1350bfcd291646e.camel@huaweicloud.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 2023-03-31 at 14:18 +0200, Roberto Sassu wrote:
> On Fri, 2023-03-31 at 09:32 +0200, Roberto Sassu wrote:
> > On Thu, 2023-03-30 at 18:55 -0400, Paul Moore wrote:
> > > On Wed, Mar 29, 2023 at 9:05 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Change the evm_inode_init_security() definition to align with the LSM
> > > > infrastructure. Keep the existing behavior of including in the HMAC
> > > > calculation only the first xattr provided by LSMs.
> > > > 
> > > > Changing the evm_inode_init_security() definition requires passing the
> > > > xattr array allocated by security_inode_init_security(), and the number of
> > > > xattrs filled by previously invoked LSMs.
> > > > 
> > > > Use the newly introduced lsm_find_xattr_slot() to position EVM correctly in
> > > > the xattrs array, like a regular LSM, and to increment the number of filled
> > > > slots. For now, the LSM infrastructure allocates enough xattrs slots to
> > > > store the EVM xattr, without using the reservation mechanism.
> > > > 
> > > > Finally, make evm_inode_init_security() return value compatible with the
> > > > inode_init_security hook conventions, i.e. return -EOPNOTSUPP if it is not
> > > > setting an xattr.
> > > > 
> > > > EVM is a bit tricky, because xattrs is both an input and an output. If it
> > > > was just output, EVM should have returned zero if xattrs is NULL. But,
> > > > since xattrs is also input, EVM is unable to do its calculations, so return
> > > > -EOPNOTSUPP and handle this error in security_inode_init_security().
> > > 
> > > I don't quite understand why EVM would return EOPNOTSUPP if it is
> > > enabled but there are not xattrs to measure.  It seems like EVM should
> > > return success/0 in the no-xattr case; there were no xattrs to
> > > measure, so it succeeded in measuring nothing.  Am I missing
> > > something?
> > 
> > From a very quick look at what other LSMs do, it seems that they return
> > zero even if they are not initialized.
> > 
> > So, it makes sense to return zero also here.
> 
> Oh, actually there was a reason to do that. If an LSM does not wish to
> provide an xattr, it should return -EOPNOTSUPP.

In general, the original purpose of -EOPNOTSUPP was to indicate that
the filesystem itself did not support security xattrs.  This can be
seen in evm_verify_hmac(), which returns different values (e.g.
INTEGRITY_NOLABEL, INTEGRITY_NOXATTRS, INTEGRITY_UNKNOWN) based on
whether security.evm or any protected security xattrs exist.

> 
> As we are not checking this convention anymore, it is probably fine to
> return zero. I already made the change, will send the new version
> shortly.

For security xattr initialization, agreed.

Mimi

> 
> > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  include/linux/evm.h               | 14 ++++++++------
> > > >  security/integrity/evm/evm_main.c | 18 +++++++++++-------
> > > >  security/security.c               |  6 +++---
> > > >  3 files changed, 22 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/include/linux/evm.h b/include/linux/evm.h
> > > > index 7dc1ee74169..3c0e8591b69 100644
> > > > --- a/include/linux/evm.h
> > > > +++ b/include/linux/evm.h
> > > > @@ -56,9 +56,10 @@ static inline void evm_inode_post_set_acl(struct dentry *dentry,
> > > >  {
> > > >         return evm_inode_post_setxattr(dentry, acl_name, NULL, 0);
> > > >  }
> > > > -extern int evm_inode_init_security(struct inode *inode,
> > > > -                                  const struct xattr *xattr_array,
> > > > -                                  struct xattr *evm);
> > > > +extern int evm_inode_init_security(struct inode *inode, struct inode *dir,
> > > > +                                  const struct qstr *qstr,
> > > > +                                  struct xattr *xattrs,
> > > > +                                  int *num_filled_xattrs);
> > > >  extern bool evm_revalidate_status(const char *xattr_name);
> > > >  extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
> > > >  extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
> > > > @@ -157,9 +158,10 @@ static inline void evm_inode_post_set_acl(struct dentry *dentry,
> > > >         return;
> > > >  }
> > > > 
> > > > -static inline int evm_inode_init_security(struct inode *inode,
> > > > -                                         const struct xattr *xattr_array,
> > > > -                                         struct xattr *evm)
> > > > +static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
> > > > +                                         const struct qstr *qstr,
> > > > +                                         struct xattr *xattrs,
> > > > +                                         int *num_filled_xattrs)
> > > >  {
> > > >         return 0;
> > > >  }
> > > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > > index cf24c525558..9e75759150c 100644
> > > > --- a/security/integrity/evm/evm_main.c
> > > > +++ b/security/integrity/evm/evm_main.c
> > > > @@ -21,6 +21,7 @@
> > > >  #include <linux/evm.h>
> > > >  #include <linux/magic.h>
> > > >  #include <linux/posix_acl_xattr.h>
> > > > +#include <linux/lsm_hooks.h>
> > > > 
> > > >  #include <crypto/hash.h>
> > > >  #include <crypto/hash_info.h>
> > > > @@ -864,23 +865,26 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> > > >  /*
> > > >   * evm_inode_init_security - initializes security.evm HMAC value
> > > >   */
> > > > -int evm_inode_init_security(struct inode *inode,
> > > > -                                const struct xattr *lsm_xattr,
> > > > -                                struct xattr *evm_xattr)
> > > > +int evm_inode_init_security(struct inode *inode, struct inode *dir,
> > > > +                           const struct qstr *qstr, struct xattr *xattrs,
> > > > +                           int *num_filled_xattrs)
> > > >  {
> > > >         struct evm_xattr *xattr_data;
> > > > +       struct xattr *evm_xattr;
> > > >         int rc;
> > > > 
> > > > -       if (!(evm_initialized & EVM_INIT_HMAC) ||
> > > > -           !evm_protected_xattr(lsm_xattr->name))
> > > > -               return 0;
> > > > +       if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs ||
> > > > +           !evm_protected_xattr(xattrs->name))
> > > > +               return -EOPNOTSUPP;
> > > > +
> > > > +       evm_xattr = lsm_find_xattr_slot(xattrs, num_filled_xattrs);
> > > > 
> > > >         xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> > > >         if (!xattr_data)
> > > >                 return -ENOMEM;
> > > > 
> > > >         xattr_data->data.type = EVM_XATTR_HMAC;
> > > > -       rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> > > > +       rc = evm_init_hmac(inode, xattrs, xattr_data->digest);
> > > >         if (rc < 0)
> > > >                 goto out;
> > > > 
> > > > diff --git a/security/security.c b/security/security.c
> > > > index be33d643a81..22ab4fb7ebf 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -1674,9 +1674,9 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> > > >         if (!num_filled_xattrs)
> > > >                 goto out;
> > > > 
> > > > -       ret = evm_inode_init_security(inode, new_xattrs,
> > > > -                                     new_xattrs + num_filled_xattrs);
> > > > -       if (ret)
> > > > +       ret = evm_inode_init_security(inode, dir, qstr, new_xattrs,
> > > > +                                     &num_filled_xattrs);
> > > > +       if (ret && ret != -EOPNOTSUPP)
> > > >                 goto out;
> > > >         ret = initxattrs(inode, new_xattrs, fs_data);
> > > >  out:
> > > > --
> > > > 2.25.1
> > > > 
>