On 18/10/2022 22:13, Beata Michalska wrote:
On Thu, Oct 13, 2022 at 10:49:52AM +0200, Kevin Brodsky wrote:
On 11/10/2022 22:08, Beata Michalska wrote:
On Wed, Sep 28, 2022 at 04:31:43PM +0100, Kevin Brodsky wrote:
For architectures implementing CHERI capabilities, introduce a new cheri.h header that defines a few helpers and macros, in addition to what is available in the compiler-provided <cheriintrin.h>.
This header is only meant for code manipulating CHERI capabilities explicitly and expands to nothing if __CHERI__ is not defined.
CONFIG_ARCH_HAS_CHERI_H is also added to allow architectures to override some of the definitions by providing their own asm/cheri.h.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/Kconfig | 3 ++ include/linux/cheri.h | 122 ++++++++++++++++++++++++++++++++++++++++++ lib/Makefile | 2 + lib/cheri.c | 67 +++++++++++++++++++++++ 4 files changed, 194 insertions(+) create mode 100644 include/linux/cheri.h create mode 100644 lib/cheri.c
diff --git a/arch/Kconfig b/arch/Kconfig index afde8a4eeee6..b173a07c5a91 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1376,6 +1376,9 @@ config DYNAMIC_SIGFRAME config HAVE_ARCH_NODE_DEV_GROUP bool +config ARCH_HAS_CHERI_H
- bool
I do appreciate that we did start with the ARCH_HAS_USER_PTR_H and current, ARCH_HAS_CHERI_H, seems like a natural follow-up on that one, though I am somewhat leaning towards switching that to HAVE_ARCH_CHERI_H to align with already existing (prior to CHERI changes) HAVE_ARCH_COMPILER_H - which does seem to expose same behaviour as the one we are after here. Not to mention we are getting rid of ARCH_HAS_USER_PTR_H. Or is there (maybe not so subtle) difference between ARCH_HAS_xxx and HAVE_ARCH_xxx ?
Honestly, no idea, I just picked ARCH_HAS_* because I liked the sound of it better :D arch/Kconfig has a bunch of both, there used to be ARCH_HAS_DMA_COHERENCE_H but it got removed last year. No concern with using HAVE_ARCH_COMPILER_H if you prefer it.
When it comes to arch-specific header files I could only spot HAVE_ARCH_COMPILER_H to be fair (looking at v6.0.2) though, using 'have' here instead of 'has' seems somehow wrong to me (?). Anyways, will leave it up to you as there seems to be no firmly established approach on how to handle it.
I don't see a problem with aligning with the only one left, i.e. HAVE_ARCH_COMPILER_H, so I'll change that.
config ARCH_HAS_USER_PTR_H bool diff --git a/include/linux/cheri.h b/include/linux/cheri.h new file mode 100644 index 000000000000..aa03edeaeaf9 --- /dev/null +++ b/include/linux/cheri.h @@ -0,0 +1,122 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef _LINUX_CHERI_H +#define _LINUX_CHERI_H
+#ifdef __CHERI__
+#include <cheriintrin.h>
+#include <linux/types.h>
+#include <uapi/asm/cheri.h> +#ifdef CONFIG_ARCH_HAS_CHERI_H +#include <asm/cheri.h> +#endif
+/*
- Standard permission sets for new capabilities. Can be overridden by
- architectures to add arch-specific permissions.
- */
+#ifndef CHERI_PERMS_READ +#define CHERI_PERMS_READ \
- (CHERI_PERM_LOAD | CHERI_PERM_LOAD_CAP)
+#endif
+#ifndef CHERI_PERMS_WRITE +#define CHERI_PERMS_WRITE \
- (CHERI_PERM_STORE | CHERI_PERM_STORE_CAP | CHERI_PERM_STORE_LOCAL_CAP)
+#endif
+#ifndef CHERI_PERMS_EXEC +#define CHERI_PERMS_EXEC \
- (CHERI_PERM_EXECUTE | CHERI_PERM_SYSTEM_REGS)
+#endif
+#ifndef CHERI_PERMS_ROOTCAP +#define CHERI_PERMS_ROOTCAP \
- (CHERI_PERM_GLOBAL | CHERI_PERM_SW_VMEM)
+#endif
What kind of variations of those are we expecting? Wouldn't it suffice to have them suffixed with smth like '_BASE' and defined unconditionally ?
No variations expected on the base sets, I was just trying to keep it simple. I could introduce new base sets that each arch can reuse when overriding, though I'm slightly concerned it could be misunderstood by users of cheri.h (because these base sets should never be used apart from the very specific use-case of arch overrides). Maybe prefixed with __? Not sure.
So what triggered my comment was the #ifdef blocks around each permission set, which got me thinking why any arch would want to provide its own definition for basic permissions.
That's because certain arch-specific permissions really have a "default on" semantics, notably MutableLoad on Morello. That is, the basic "read" set for Morello is Load + LoadCap + MutableLoad, as per patch 5. That's why I'm not sure about the usefulness of "base base" permission sets, which should never be used outside of arch-specific overrides. Since I would like the standard permission sets to remain defined here (instead of forcing each arch to define them explicitly), it does not really reduce duplication either.
So what I was suggesting here was to have those defined unconditionally with the assumptions it can be safely re-used by arch specific code when defining more complex permission sets , as of:
#define CHERI_PERMS_READ_BASE
We could go with your solution and having it as:
#define __CHERI_PERMS_READ
as long as it's safe (and it should (?)) to assume it will mean the same for all archs potentially using this header. (which now makes me wondering about the CHERI_PERMS_ROOTCAP one ...) Either way I would lean towards using the prefix you have suggested.
+/**
- cheri_build_user_cap() - Create a userspace capability.
- @addr: Requested capability address.
- @len: Requested capability length.
- @perms: Requested capability permissions.
- Returns a new capability derived from the root userspace capability. Its
Minor: As we intend(?) to align with the Documentation/doc-guide/kernel-doc.rst this one should probably be: Returns: A new capability ...... Same applies to the remaining ones.
Done while looking at patch 2.
- address and permissions are set according to @addr and @perms respectively.
- Its bounds are set exactly with @addr as base address and @len as length.
- The caller is responsible to ensure that:
- @addr is a valid userspace address.
- The (@addr, @len) tuple can be represented as capability bounds.
- @perms are valid permissions for a userspace capability.
- If either 1. or 2. does not hold, the resulting capability will be invalid.
- If 3. does not hold, the returned capability will not have any of the invalid
- permissions.
- */
+void * __capability +cheri_build_user_cap(ptraddr_t addr, size_t len, cheri_perms_t perms);
+/**
- cheri_build_user_cap_inexact_bounds() - Create a userspace capability,
- allowing bounds to be enlarged.
- @addr: Requested capability address.
- @len: Requested capability length.
- @perms: Requested capability permissions.
- Returns a new capability derived from the root userspace capability. Its
- address and permissions are set according to @addr and @perms respectively.
- Its bounds are set to the smallest representable range that includes the
- range [@addr, @addr + @len[.
- This variant of cheri_build_user_cap() should only be used when it is safe to
- enlarge the bounds of the capability. In particular, it should never be used
- when creating a capability that is to be provided to userspace, because the
- potentially enlarged bounds might give access to unrelated objects.
- The caller is responsible to ensure that:
- @addr is a valid userspace address.
- @perms are valid permissions for a userspace capability.
- If 1. does not hold, the resulting capability will be invalid.
- If 2. does not hold, the returned capability will not have any of the invalid
- permissions.
- */
+void * __capability +cheri_build_user_cap_inexact_bounds(ptraddr_t addr, size_t len,
cheri_perms_t perms);How about flipping the naming a bit here and have this one as : cheri_build_user_cap and the one above as cheri_build_user_cap_exact, to align with what compiler is doing with the cheri builtins (namely __builtin_cheri_bounds_set and __builtin_cheri_bounds_set_exact) ?
It would be less wonky for sure, but I did this entirely on purpose. I want to make it crystal clear that code is using the inexact variant, because this is the dangerous one from the perspective of the capability model: it can return a capability whose bounds are larger than the object you're intending to give access to, which is almost never fine (without careful processing) if the capability is then returned to userspace. That's the same rationale as for the naming of make_privileged_user_ptr() though I'll come back to this in patch 8.
That does make sense! How about making it more verbose, as of : cheri_build_user_cap_unsafe, which should already scream to pay attention to the outcome, without the specifics on the nature of it being unsafe ? Just a suggestion though.
That's an option, though for the cheri.h interface I'd rather be precise on the naming as this is meant for "specialist" code (doing low-level CHERI stuff). We could say "unsafe_inexact_bounds" but that's probably a bit much :)
+/**
- cheri_check_cap_data_access() - Check whether a capability gives access to a
- range of addresses.
- @cap: Capability to check.
- @len: Length of the access.
- @perms: Required permissions.
- Returns true if the capability gives access to a given range of addresses
- and has the requested permissions. This means that:
- @cap is valid and unsealed.
- The range [@cap.address, @cap.address + @len[ is within the bounds
- of @cap.
- The permissions of @cap include at least @perms.
- */
+bool cheri_check_cap_data_access(void * __capability cap, size_t len,
cheri_perms_t perms);Wouldn't cheri_check_cap_access be sufficient ? Or cheri_check_access even, as it is somewhat obvious it will be operating on a capability ? On a second thought, as there is access_ok already, how about simply cheri_access_ok ?
See my reply to Amit on this same patch regarding "data". Agreed "cap" is somewhat obvious, though I would only remove it if "data" is removed too as this looks a bit ambiguous to me otherwise. I would avoid cheri_access_ok() as it opens the can of worms of "why don't you just do this in access_ok()" (while here we only need this function when we cannot just dereference the pointer and let it fault).
On the 'access_ok' front I did miss the context here where we cannot handle that in 'can fault' section. Further comments on the other thread to ease the review process.
+/*
- Root capabilities. Should be set in arch code during the early init phase,
- read-only after that.
- The helpers above should be used instead where possible.
- */
+extern uintcap_t cheri_root_cap_userspace; +extern uintcap_t cheri_root_seal_cap_userspace; +extern uintcap_t cheri_root_cid_cap_userspace;
For some reason (and those are just personal preferences here) I am not entirely convinced with the naming here: maybe cheri_user_root_xxx instead of appending the 'userspace' ? Although not much of a difference there in the end.
I did ponder on this too and had a hard time making a decision. In the end I tried to build the name in a hierarchical way, that is: "cheri" (module) + "root*_cap" (nature of the object) + "userspace" (which object exactly). We will probably end up with kernel variants too, so the question is also which of "cheri_root_cap_kernel" or "cheri_kernel_root_cap" makes more sense. Maybe the latter just looks better?
Both versions make sense, but I agree the latter just looks/reads better.
Will change accordingly.
Also, it took me a moment to translate cid to compartment ID :)
Also hesitated on this one but I just aligned it on AT_CHERI_CID_CAP - cheri_root_compartmentid_cap_userspace would be quite a mouthful!
Having the cid here is perfectly fine, using the fully elaborated naming here is far from being perfect. I think me being lazy in nature, I was looking for a comment maybe ? saying cid == compartment id , especially that this is the first time it pops up for the kernel itself.
Will add a comment, that can't hurt.
Kevin
Anyways, very much happy with having this as cheri_***_cid__****
BR B.
Kevin
BR B.
+#endif /* __CHERI__ */
+#endif /* _LINUX_CHERI_H */ diff --git a/lib/Makefile b/lib/Makefile index 6b9ffc1bd1ee..8dd8c00fee31 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -263,6 +263,8 @@ obj-$(CONFIG_MEMREGION) += memregion.o obj-$(CONFIG_STMP_DEVICE) += stmp_device.o obj-$(CONFIG_IRQ_POLL) += irq_poll.o +obj-y += cheri.o
- # stackdepot.c should not be instrumented or call instrumented functions. # Prevent the compiler from calling builtins like memcmp() or bcmp() from this # file.
diff --git a/lib/cheri.c b/lib/cheri.c new file mode 100644 index 000000000000..4fe8e9e7f72c --- /dev/null +++ b/lib/cheri.c @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifdef __CHERI__
+#include <linux/bug.h> +#include <linux/cheri.h> +#include <linux/mm.h>
+uintcap_t cheri_root_cap_userspace __ro_after_init; +uintcap_t cheri_root_seal_cap_userspace __ro_after_init; +uintcap_t cheri_root_cid_cap_userspace __ro_after_init;
+static void * __capability +build_user_cap(ptraddr_t addr, size_t len, cheri_perms_t perms, bool exact_bounds) +{
- void * __capability ret = (void * __capability)cheri_root_cap_userspace;
- cheri_perms_t root_perms = cheri_perms_get(ret);
- ret = cheri_perms_and(ret, perms);
- ret = cheri_address_set(ret, addr);
- if (exact_bounds)
ret = cheri_bounds_set_exact(ret, len);- else
ret = cheri_bounds_set(ret, len);- WARN(perms & ~root_perms,
"Permission mask %#lx discarded while creating user capability %#lp\n",perms & ~root_perms, ret);- WARN(cheri_is_invalid(ret),
"Invalid user capability created: %#lp (%s bounds requested)\n",ret, (exact_bounds ? "exact" : "inexact"));- return ret;
+}
+void * __capability +cheri_build_user_cap(ptraddr_t addr, size_t len, cheri_perms_t perms) +{
- return build_user_cap(addr, len, perms, true);
+}
+void * __capability +cheri_build_user_cap_inexact_bounds(ptraddr_t addr, size_t len,
cheri_perms_t perms)+{
- return build_user_cap(addr, len, perms, false);
+}
+bool cheri_check_cap_data_access(void * __capability cap, size_t len,
cheri_perms_t perms)+{
- ptraddr_t addr = untagged_addr(cheri_address_get(cap));
- ptraddr_t base = cheri_base_get(cap); /* Never tagged */
- if (cheri_is_invalid(cap) || cheri_is_sealed(cap))
return false;- if (addr < base || addr > base + cheri_length_get(cap) - len)
return false;- if (perms & ~cheri_perms_get(cap))
return false;- return true;
+}
+#endif /* __CHERI__ */
2.34.1
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org