Received-SPF: pass (google.com: domain of linux-kernel+bounces-90924-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a7124afa6bed2fcadcb66efa08e256828cd6f8ab.camel@huaweicloud.com>
Subject: Re: [PATCH v2 24/25] commoncap: use vfs fscaps interfaces
From: Roberto Sassu <roberto.sassu@huaweicloud.com>
To: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>, Serge Hallyn <serge@hallyn.com>,
  Paul Moore <paul@paul-moore.com>, Eric Paris <eparis@redhat.com>, James
 Morris <jmorris@namei.org>,  Alexander Viro <viro@zeniv.linux.org.uk>, Jan
 Kara <jack@suse.cz>, Stephen Smalley <stephen.smalley.work@gmail.com>,
 Ondrej Mosnacek <omosnace@redhat.com>,  Casey Schaufler
 <casey@schaufler-ca.com>, Mimi Zohar <zohar@linux.ibm.com>, Roberto Sassu
 <roberto.sassu@huawei.com>,  Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
 Eric Snowberg <eric.snowberg@oracle.com>, "Matthew Wilcox (Oracle)"
 <willy@infradead.org>, Jonathan Corbet <corbet@lwn.net>, Miklos Szeredi
 <miklos@szeredi.hu>, Amir Goldstein <amir73il@gmail.com>, 
 linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, 
 linux-security-module@vger.kernel.org, audit@vger.kernel.org, 
 selinux@vger.kernel.org, linux-integrity@vger.kernel.org, 
 linux-doc@vger.kernel.org, linux-unionfs@vger.kernel.org
Date: Mon, 04 Mar 2024 17:17:57 +0100
In-Reply-To: <ZeXpbOsdRTbLsYe9@do-x1extreme>
References: <20240221-idmap-fscap-refactor-v2-0-3039364623bd@kernel.org>
	 <20240221-idmap-fscap-refactor-v2-24-3039364623bd@kernel.org>
	 <dcbd9e7869d2fcce69546b53851d694b8ebad54e.camel@huaweicloud.com>
	 <ZeXpbOsdRTbLsYe9@do-x1extreme>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-03-04 at 09:31 -0600, Seth Forshee (DigitalOcean) wrote:
> On Mon, Mar 04, 2024 at 11:19:54AM +0100, Roberto Sassu wrote:
> > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote:
> > > Use the vfs interfaces for fetching file capabilities for killpriv
> > > checks and from get_vfs_caps_from_disk(). While there, update the
> > > kerneldoc for get_vfs_caps_from_disk() to explain how it is different
> > > from vfs_get_fscaps_nosec().
> > >=20
> > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
> > > ---
> > >  security/commoncap.c | 30 +++++++++++++-----------------
> > >  1 file changed, 13 insertions(+), 17 deletions(-)
> > >=20
> > > diff --git a/security/commoncap.c b/security/commoncap.c
> > > index a0ff7e6092e0..751bb26a06a6 100644
> > > --- a/security/commoncap.c
> > > +++ b/security/commoncap.c
> > > @@ -296,11 +296,12 @@ int cap_capset(struct cred *new,
> > >   */
> > >  int cap_inode_need_killpriv(struct dentry *dentry)
> > >  {
> > > -	struct inode *inode =3D d_backing_inode(dentry);
> > > +	struct vfs_caps caps;
> > >  	int error;
> > > =20
> > > -	error =3D __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
> > > -	return error > 0;
> > > +	/* Use nop_mnt_idmap for no mapping here as mapping is unimportant =
*/
> > > +	error =3D vfs_get_fscaps_nosec(&nop_mnt_idmap, dentry, &caps);
> > > +	return error =3D=3D 0;
> > >  }
> > > =20
> > >  /**
> > > @@ -323,7 +324,7 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, s=
truct dentry *dentry)
> > >  {
> > >  	int error;
> > > =20
> > > -	error =3D __vfs_removexattr(idmap, dentry, XATTR_NAME_CAPS);
> > > +	error =3D vfs_remove_fscaps_nosec(idmap, dentry);
> >=20
> > Uhm, I see that the change is logically correct... but the original
> > code was not correct, since the EVM post hook is not called (thus the
> > HMAC is broken, or an xattr change is allowed on a portable signature
> > which should be not).
> >=20
> > For completeness, the xattr change on a portable signature should not
> > happen in the first place, so cap_inode_killpriv() would not be called.
> > However, since EVM allows same value change, we are here.
>=20
> I really don't understand EVM that well and am pretty hesitant to try an
> change any of the logic around it. But I'll hazard a thought: should EVM
> have a inode_need_killpriv hook which returns an error in this
> situation?

Uhm, I think it would not work without modifying
security_inode_need_killpriv() and the hook definition.

Since cap_inode_need_killpriv() returns 1, the loop stops and EVM would
not be invoked. We would need to continue the loop and let EVM know
what is the current return value. Then EVM can reject the change.

An alternative way would be to detect that actually we are setting the
same value for inode metadata, and maybe not returning 1 from
cap_inode_need_killpriv().

I would prefer the second, since EVM allows same value change and we
would have an exception if there are fscaps.

This solves only the case of portable signatures. We would need to
change cap_inode_need_killpriv() anyway to update the HMAC for mutable
files.

Roberto