On Thu, Feb 02, 2023 at 11:20:56AM +0100, Kevin Brodsky wrote:
On 31/01/2023 12:25, Beata Michalska wrote:
diff --git a/arch/arm64/kernel/morello.c b/arch/arm64/kernel/morello.c index ccbf26e77919..0bb7b3dbedec 100644 --- a/arch/arm64/kernel/morello.c +++ b/arch/arm64/kernel/morello.c @@ -5,10 +5,9 @@ #define pr_fmt(fmt) "morello: " fmt -#include <cheriintrin.h>
#include <linux/cache.h> #include <linux/capability.h> +#include <linux/cheri.h> #include <linux/compat.h> #include <linux/mm.h> #include <linux/printk.h> @@ -21,10 +20,6 @@ #include <asm/morello.h> #include <asm/ptrace.h> -#ifdef CONFIG_CHERI_PURECAP_UABI -#include <cheriintrin.h> -#endif
/* Private functions implemented in morello.S */ void __morello_cap_lo_hi_tag(uintcap_t cap, u64 *lo_val, u64 *hi_val, u8 *tag); @@ -295,8 +290,8 @@ static void __init check_root_cap(uintcap_t cap) __morello_cap_lo_hi_tag(cap, &lo_val, &hi_val, &tag); /*
* Check that DDC has the reset value, otherwise morello_root_cap and* all capabilities derived from it (especially those exposed to
* Check that DDC has the reset value, otherwise root capabilities and* all capabilities derived from them (notably those exposed to*/ if (!(tag == 1 &&
- userspace) may not be reliable.
@@ -307,17 +302,52 @@ static void __init check_root_cap(uintcap_t cap) static int __init morello_cap_init(void) { -#ifdef CONFIG_CHERI_PURECAP_UABI
- uintcap_t ctemp;
-#endif
- uintcap_t root_cap, user_root_all_cap, ctemp;
- root_cap = (uintcap_t)cheri_ddc_get();
- check_root_cap(root_cap);
- /* Initialise standard CHERI root capabilities. */
- ctemp = cheri_address_set(root_cap, 0);
- /* Same upper limit as for access_ok() and __uaccess_mask_ptr() */
- ctemp = cheri_bounds_set(ctemp, TASK_SIZE_MAX);
Out of curiosity : why not *_set_exact ?
Because in principle, that would require checking that the range is representable. If somehow it wasn't we would have no choice but to expand it to make it representable, which is exactly the same as using the inexact variant. OTOH if we assume that the range is indeed representable, then using one or the other makes no difference.
Thanks for clarifying.
- ctemp = cheri_perms_and(ctemp, CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ |
CHERI_PERMS_WRITE | CHERI_PERMS_EXEC |ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR |CHERI_PERM_SEAL | CHERI_PERM_UNSEAL |ARM_CAP_PERMISSION_COMPARTMENT_ID);- user_root_all_cap = ctemp;
- cheri_user_root_all_cap = user_root_all_cap;
- ctemp = user_root_all_cap;
I think this one is spurious (?): ctemp has not changed since the assignment two lines above.
See Amit's comment and my reply, I thought I'd introduce a local variable to avoid continuously reading a global, but in fact Clang is very happy to optimise that out too, so I've got rid of user_root_all_cap altogether.
- ctemp = cheri_perms_and(ctemp, CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ |
CHERI_PERMS_WRITE | CHERI_PERMS_EXEC |ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR);- cheri_user_root_cap = ctemp;
This will probably be optimized out but it could be combined with the cheri_perms_and above and you could drop the following two assignments. Similar optimization could also be done below.
This is certainly optimised out, such local variables are only a matter of style. I've thought about a number of options but in the end it feels like using a temporary for all operations is more consistent, though it does look a bit weird when there is only one operation (i.e. setting permissions). Given the length of the globals' names, I'm not sure doing it in one statement is clearer though.
Thinking some more, we could get a little fancy and introduce some helper to set permissions and bounds. Since we don't always need to set the bounds, that argument should be optional. Conveniently we've found a good macro trick to do that in LTP, so we could reuse it here, something like:
static uintcap_t __build_cap(uintcap_t root, cheri_perms_t perms, size_t length) { uintcap_t c = root;
c = cheri_perms_and(c, perms); if (length) c = cheri_bounds_set(c, length);
return c; } #define build_cap(root, perms, ...) __build_cap(root, perms, ##__VA_ARGS__, 0)
Then to keep it readable, I would use a temporary for the permissions, something like this:
cheri_perms_t perms; perms = CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | CHERI_PERMS_WRITE | CHERI_PERMS_EXEC | ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR; cheri_user_root_cap = build_cap(cheri_user_root_all_cap, perms);
How does that sound?
Sounds great actually.
--- BR B.
Kevin
BR B.
- ctemp = user_root_all_cap;
- /*
* All object types, not a final decision - some of them may be later* reserved to the kernel.*/- ctemp = cheri_bounds_set(ctemp, 1u << 15);
- ctemp = cheri_perms_and(ctemp, CHERI_PERM_GLOBAL |
CHERI_PERM_SEAL | CHERI_PERM_UNSEAL);- cheri_user_root_seal_cap = ctemp;
- morello_root_cap = (uintcap_t)cheri_ddc_get();
- /* Maximum userspace bounds for the time being. */
- ctemp = user_root_all_cap;
- ctemp = cheri_perms_and(ctemp, CHERI_PERM_GLOBAL |
ARM_CAP_PERMISSION_COMPARTMENT_ID);- cheri_user_root_cid_cap = ctemp;
- check_root_cap(morello_root_cap);
- /* Initialise Morello-specific root capabilities. */
- morello_root_cap = root_cap;
#ifdef CONFIG_CHERI_PURECAP_UABI /* Initialize a capability able to unseal sentry capabilities. */
- ctemp = cheri_address_set(morello_root_cap, CHERI_OTYPE_SENTRY);
- ctemp = cheri_address_set(root_cap, CHERI_OTYPE_SENTRY); ctemp = cheri_bounds_set(ctemp, 1); ctemp = cheri_perms_and(ctemp, CHERI_PERM_GLOBAL | CHERI_PERM_UNSEAL); morello_sentry_unsealcap = ctemp;