From: Paul Moore <paul@paul-moore.com>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
Mimi Zohar <zohar@linux.ibm.com>,
linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org
Subject: Re: [PATCH v2 18/30] evm: simplify evm_xattr_acl_change()
Date: Tue, 27 Sep 2022 18:56:44 -0400 [thread overview]
Message-ID: <CAHC9VhTx-Pkh0E3Awr=BR-Zh31gmoP3d1MKHf-UPVibfV3VxKQ@mail.gmail.com> (raw)
In-Reply-To: <20220926140827.142806-19-brauner@kernel.org>
On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The posix acl api provides a dedicated security and integrity hook for
> setting posix acls. This means that
>
> evm_protect_xattr()
> -> evm_xattr_change()
> -> evm_xattr_acl_change()
>
> is now only hit during vfs_remove_acl() at which point we are guaranteed
> that xattr_value and xattr_value_len are NULL and 0. In this case evm
> always used to return 1. Simplify this function to do just that.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
> /* v2 */
> unchanged
>
> security/integrity/evm/evm_main.c | 62 +++++++------------------------
> 1 file changed, 14 insertions(+), 48 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 15aa5995fff4..1fbe1b8d0364 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
>
> /*
> * evm_xattr_acl_change - check if passed ACL changes the inode mode
> - * @mnt_userns: user namespace of the idmapped mount
> - * @dentry: pointer to the affected dentry
> * @xattr_name: requested xattr
> * @xattr_value: requested xattr value
> * @xattr_value_len: requested xattr value length
> *
> - * Check if passed ACL changes the inode mode, which is protected by EVM.
> + * This is only hit during xattr removal at which point we always return 1.
> + * Splat a warning in case someone managed to pass data to this function. That
> + * should never happen.
> *
> * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
> */
> -static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
> - struct dentry *dentry, const char *xattr_name,
> - const void *xattr_value, size_t xattr_value_len)
> +static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
> {
> -#ifdef CONFIG_FS_POSIX_ACL
> - umode_t mode;
> - struct posix_acl *acl = NULL, *acl_res;
> - struct inode *inode = d_backing_inode(dentry);
> - int rc;
> -
> - /*
> - * An earlier comment here mentioned that the idmappings for
> - * ACL_{GROUP,USER} don't matter since EVM is only interested in the
> - * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
> - * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
> - * representation it should do so correctly. There's no guarantee that
> - * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
> - * for the mode at some point and it's difficult to keep track of all
> - * the LSM and integrity modules and what they do to POSIX ACLs.
> - *
> - * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
> - * ACLs it received. It requires knowledge that only the VFS is
> - * guaranteed to have.
> - */
> - acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
> - xattr_value, xattr_value_len);
> - if (IS_ERR_OR_NULL(acl))
> - return 1;
> -
> - acl_res = acl;
> - /*
> - * Passing mnt_userns is necessary to correctly determine the GID in
> - * an idmapped mount, as the GID is used to clear the setgid bit in
> - * the inode mode.
> - */
> - rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
> -
> - posix_acl_release(acl);
> -
> - if (rc)
> - return 1;
> + int rc = 0;
>
> - if (inode->i_mode != mode)
> - return 1;
> +#ifdef CONFIG_FS_POSIX_ACL
> + WARN_ONCE(xattr_value != NULL,
> + "Passing xattr value for POSIX ACLs not supported\n");
> + WARN_ONCE(xattr_value_len != 0,
> + "Passing non-zero length for POSIX ACLs not supported\n");
> + rc = 1;
> #endif
> - return 0;
> +
> + return rc;
> }
This is another case where I'll leave the final say up to Mimi, but
why not just get rid of evm_xattr_acl_change() entirely? Unless I'm
missing something, it's only reason for existing now is to check that
it is passed the proper (empty) parameters which seems pointless ...
no?
--
paul-moore.com
next prev parent reply other threads:[~2022-09-27 22:57 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-26 14:07 [PATCH v2 00/30] acl: add vfs posix acl api Christian Brauner
2022-09-26 14:08 ` [PATCH v2 11/30] selinux: implement set acl hook Christian Brauner
2022-09-27 22:55 ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 13/30] evm: " Christian Brauner
2022-09-27 22:56 ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 15/30] evm: add post " Christian Brauner
2022-09-27 22:56 ` Paul Moore
2022-09-26 14:08 ` [PATCH v2 18/30] evm: simplify evm_xattr_acl_change() Christian Brauner
2022-09-27 22:56 ` Paul Moore [this message]
2022-09-28 13:31 ` Christian Brauner
2022-09-27 0:22 ` [PATCH v2 00/30] acl: add vfs posix acl api Casey Schaufler
2022-09-27 7:41 ` Christoph Hellwig
2022-09-27 7:59 ` Christian Brauner
2022-09-27 14:11 ` Casey Schaufler
2022-09-27 15:16 ` Seth Forshee
2022-09-27 15:55 ` Casey Schaufler
2022-09-27 23:24 ` Paul Moore
2022-09-27 23:37 ` Casey Schaufler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHC9VhTx-Pkh0E3Awr=BR-Zh31gmoP3d1MKHf-UPVibfV3VxKQ@mail.gmail.com' \
--to=paul@paul-moore.com \
--cc=brauner@kernel.org \
--cc=hch@lst.de \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=sforshee@kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).