| Message ID | 20231129-idmap-fscap-refactor-v1-4-da5a26058a5b@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
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
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; }
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(-)