Re: [PATCH 4/16] capability: use vfsuid_t for vfs_caps rootids

From: Paul Moore <paul@paul-moore.com>
To: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>,
	Christian Brauner <brauner@kernel.org>,
	Serge Hallyn <serge@hallyn.com>, Eric Paris <eparis@redhat.com>,
	James Morris <jmorris@namei.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Amir Goldstein <amir73il@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, audit@vger.kernel.org,
	linux-unionfs@vger.kernel.org,
	"Seth Forshee (DigitalOcean)" <sforshee@kernel.org>
Subject: Re: [PATCH 4/16] capability: use vfsuid_t for vfs_caps rootids
Date: Tue, 05 Dec 2023 16:25:27 -0500	[thread overview]
Message-ID: <c88dd2922f2689e2ede5bbf4a0e43a0a@paul-moore.com> (raw)
In-Reply-To: <20231129-idmap-fscap-refactor-v1-4-da5a26058a5b@kernel.org>

On Nov 29, 2023 "Seth Forshee (DigitalOcean)" <sforshee@kernel.org> wrote:
> 
> The rootid is a kuid_t, but it contains an id which maped into a mount
> idmapping, so it is really a vfsuid. This is confusing and creates
> potential for misuse of the value, so change it to vfsuid_t.
> 
> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
> ---
>  include/linux/capability.h | 3 ++-
>  kernel/auditsc.c           | 5 +++--
>  security/commoncap.c       | 2 +-
>  3 files changed, 6 insertions(+), 4 deletions(-)

It might be nice if AS_KUIDT() and friends were named in such a way
as to indicate that they require a vfsuid_t parameter.  At least the
call to __vfsuid_val() should flag a type mismatch if some other type
is used.  Regardless, that is more of a general VFS issue and not a
problem specific to this patchset.

With the same understanding about the capabilities code and Serge ...

Acked-by: Paul Moore <paul@paul-moore.com> (Audit,LSM)

> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index c24477e660fc..eb46d346bbbc 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -16,6 +16,7 @@
>  #include <uapi/linux/capability.h>
>  #include <linux/uidgid.h>
>  #include <linux/bits.h>
> +#include <linux/vfsid.h>
>  
>  #define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
>  
> @@ -26,7 +27,7 @@ typedef struct { u64 val; } kernel_cap_t;
>  /* same as vfs_ns_cap_data but in cpu endian and always filled completely */
>  struct vfs_caps {
>  	__u32 magic_etc;
> -	kuid_t rootid;
> +	vfsuid_t rootid;
>  	kernel_cap_t permitted;
>  	kernel_cap_t inheritable;
>  };
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 783d0bf69ca5..65691450b080 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -65,6 +65,7 @@
>  #include <uapi/linux/netfilter/nf_tables.h>
>  #include <uapi/linux/openat2.h> // struct open_how
>  #include <uapi/linux/fanotify.h>
> +#include <linux/mnt_idmapping.h>
>  
>  #include "audit.h"
>  
> @@ -2260,7 +2261,7 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>  	name->fcap.permitted = caps.permitted;
>  	name->fcap.inheritable = caps.inheritable;
>  	name->fcap.fE = !!(caps.magic_etc & VFS_CAP_FLAGS_EFFECTIVE);
> -	name->fcap.rootid = caps.rootid;
> +	name->fcap.rootid = AS_KUIDT(caps.rootid);
>  	name->fcap_ver = (caps.magic_etc & VFS_CAP_REVISION_MASK) >>
>  				VFS_CAP_REVISION_SHIFT;
>  
> @@ -2816,7 +2817,7 @@ int __audit_log_bprm_fcaps(struct linux_binprm *bprm,
>  	ax->fcap.permitted = vcaps.permitted;
>  	ax->fcap.inheritable = vcaps.inheritable;
>  	ax->fcap.fE = !!(vcaps.magic_etc & VFS_CAP_FLAGS_EFFECTIVE);
> -	ax->fcap.rootid = vcaps.rootid;
> +	ax->fcap.rootid = AS_KUIDT(vcaps.rootid);
>  	ax->fcap_ver = (vcaps.magic_etc & VFS_CAP_REVISION_MASK) >> VFS_CAP_REVISION_SHIFT;
>  
>  	ax->old_pcap.permitted   = old->cap_permitted;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cf130d81b8b4..3d045d377e5e 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -710,7 +710,7 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap,
>  	cpu_caps->permitted.val &= CAP_VALID_MASK;
>  	cpu_caps->inheritable.val &= CAP_VALID_MASK;
>  
> -	cpu_caps->rootid = vfsuid_into_kuid(rootvfsuid);
> +	cpu_caps->rootid = rootvfsuid;
>  
>  	return 0;
>  }
> -- 
> 2.43.0

--
paul-moore.com