On Thu, Oct 13, 2022 at 03:36:04PM +0200, Kevin Brodsky wrote:
On 11/10/2022 21:56, Beata Michalska wrote:
On Wed, Sep 28, 2022 at 04:31:47PM +0100, Kevin Brodsky wrote:
Introduce new helpers to operate on capability metadata in PCuABI:
- make_privileged_user_ptr() creates a user capability with the requested bounds and permissions.
- check_user_ptr() checks that the capability is valid and its bounds and permissions are appropriate for the requested access.
In the PCuABI case, these functions are implemented on top of the new cheri_* helpers in linux/cheri.h. The implementation is trivial in the !PCuABI case (the provided length and permissions are ignored).
make_privileged_user_ptr() supersedes uaddr_to_user_ptr_safe() as it creates safer capabilities for the kernel to manipulate.
Note that check_user_ptr() is only meant to be used when explicit checking is actually required. In particular, it is not needed when a user pointer is passed to uaccess functions and otherwise treated as an opaque value, which is the vast majority of cases.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
include/linux/user_ptr.h | 64 ++++++++++++++++++++++++++++++++++++++++ lib/user_ptr.c | 33 +++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 21cc0f6e1832..2a830a7f7449 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -4,6 +4,11 @@ #include <linux/typecheck.h> +typedef int user_ptr_perms_t;
+#define USER_PTR_CAN_READ 0x1 +#define USER_PTR_CAN_WRITE 0x2
I'm probably missing smth but why do we need those additional permissions on top of capability ones? Is it about simply abstracting those ?
It is indeed, this interface is completely generic.
If so, why we cannot stick to PROT_READ / PROT_WRITE (or VM_READ / VM_WRITE)? We are getting another layer of translation and I am bit concerned that this might get confusing at some point.
I did ponder on this too. I didn't really like introducing new flags for that reason. However, I am quite concerned that using PROT_*, or even worse VM_*, could suggest these flags have something to do with the memory the pointer refers to (i.e. whether memory mappings are configured in such a way that say a read can be made if PROT_READ is passed), when in fact this is entirely unrelated. These flags are about whether the pointer allows reading/writing, no more, no less. I feel that being explicit is important here as the overlay of capability permissions on top of MMU permissions can create a lot of confusion.
I kinda see where you are going with that. Do you forsee those being used more widely beyond what has been suggested here ? I'm just wondering if this could be solved by specifying right handlers instead of having those additional flags like: make_privileged_user_ptr_read/write/rw ?
which would be simple wrappers around make_privileged_user_ptr providing expected permission bits (skipping the naming for the time being) ? This would allow us to drop yet another abstraction on permissions and the additional typedef, and could potentially be more suggestive as per what do we mean by permissions here ? I know I am being extra peaky here (and apologies for that), but for pointer-type capabilities CHERI permissions do end up determining what memory access is allowed. And the functionality here does consider pointer-type capabilities only, so the additional abstraction level does seem superficial.
Aside: what is the use case for having capability permissions being wider than memory access ones ? (apologies if there is an obvious answer that I am missing at this point, apparently I already got confused by CHERI permissions vs MMU ones)
Overall, my rambling here might be just that - pure rambling.
And what about execute permission?
Another wilful omission :) In this case it's less about principles and more about the fact that I can't think of any situation where we need to create an executable user pointer for in-kernel usage, or check that a user pointer is executable. The former seems very unlikely to change, and the latter could potentially happen, but we would anyway have to handle it in a special way (because function pointers are sealed). In other words: let's cross that bridge if and when we get there.
Noted - makes sense.
/**
- as_user_ptr() - Convert an arbitrary integer value to a user pointer.
- @x: The integer value to convert.
@@ -51,9 +56,56 @@ void __user *uaddr_to_user_ptr(ptraddr_t addr);
- This function should be used when a user pointer is required because user
- memory at a certain address needs to be accessed, and that address originates
- from the kernel itself (i.e. it is not provided by userspace).
- Deprecated in favour of make_privileged_user_ptr(), which provides more
*/ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr);
- precise information for the pure-capability uABI.
+/**
- make_privileged_user_ptr() - Create a user pointer from kernel-generated
- parameters, for in-kernel use only.
- @addr: The address to set the pointer to.
- @len: The minimum size of the region the pointer should allow to access.
- @perms: The type of operation the pointer should allow; bitwise combination
of USER_PTR_CAN_*.
- Returns a user pointer with its address set to @addr.
- This function should be used when a user pointer is required because user
- memory at a certain address needs to be accessed, and that address originates
- from the kernel itself. Additionally, the returned user pointer should only
- be used by the kernel itself. In other words, the parameters should not be
- provided by userspace, and the returned pointer should not be provided to
- userspace in any way.
- When the pure-capability uABI is targeted, the returned capability pointer
- will have its length set to at least @len (the base and length may be
- expanded because of representability constraints), and its permissions will
- be set according to @perms.
- */
+void __user *make_privileged_user_ptr(ptraddr_t addr, size_t len,
user_ptr_perms_t perms);I'm having mixed feelings about the naming here: 'make_privileged' is somehow suggesting that we grant some rights that otherwise are not there, and this one seems to be simply creating a valid capability based on requested access, which, as I assume, should be already in place for the memory at hand. So what we are doing here is basically mapping memory access permissions to capability metadata. So it's not increasing any privileges per se, just providing different view on those. Unless, I have completely misunderstood the intention here.
Overall this is correct, with a subtlety on the bounds precision. See my reply to Amit's email.
To be continued there then...
+/**
- check_user_ptr() - Check whether a user pointer grants access to a memory
- region.
- @ptr: The pointer to check.
- @len: The size of the region that needs to be accessible.
- @perms: The type of operation the pointer needs to allow; bitwise combination
of USER_PTR_CAN_*.
- Returns whether @ptr allows accessing the memory region starting at the
- address of @ptr and of size @len. The type of access that @ptr should allow
- is specified by @perms.
- This function only checks whether the **pointer itself** allows a given
- access; no other check is performed. Such checks are only performed when user
- pointers have appropriate metadata, as in the pure-capability uABI, otherwise
- the function always returns true.
- */
+bool check_user_ptr(void __user *ptr, size_t len, user_ptr_perms_t perms);
- #else /* CONFIG_CHERI_PURECAP_UABI */ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr)
@@ -66,6 +118,18 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) return as_user_ptr(addr); } +static inline void __user *make_privileged_user_ptr(ptraddr_t addr, size_t len,
user_ptr_perms_t perms)+{
- return as_user_ptr(addr);
+}
+static inline bool check_user_ptr(void __user *ptr, size_t len,
user_ptr_perms_t perms)+{
- return true;
+}
- #endif /* CONFIG_CHERI_PURECAP_UABI */ /**
diff --git a/lib/user_ptr.c b/lib/user_ptr.c index 503a8369d019..826996cd45bd 100644 --- a/lib/user_ptr.c +++ b/lib/user_ptr.c @@ -27,3 +27,36 @@ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) return ret; } +void __user *make_privileged_user_ptr(ptraddr_t addr, size_t len,
user_ptr_perms_t perms)+{
- cheri_perms_t cap_perms = CHERI_PERM_GLOBAL;
- /*
* Grant all permissions in each category, e.g. loading/storing* capabilities in addition to standard data.*/- if (perms & USER_PTR_CAN_READ)
cap_perms |= CHERI_PERMS_READ;- if (perms & USER_PTR_CAN_WRITE)
cap_perms |= CHERI_PERMS_WRITE;- return cheri_build_user_cap_inexact_bounds(addr, len, cap_perms);
+}
+bool check_user_ptr(void __user *ptr, size_t len, user_ptr_perms_t perms) +{
- cheri_perms_t cap_perms = 0;
- /*
* Only check whether the capability has the minimal data permissions* (Load / Store). The underlying assumption is that this function is* only used before user data pages are grabbed via GUP and the data is* then copied through a kernel mapping, and does not contain* capabilities.*/- if (perms & USER_PTR_CAN_READ)
cap_perms |= CHERI_PERM_LOAD;- if (perms & USER_PTR_CAN_WRITE)
cap_perms |= CHERI_PERM_STORE;I think it would be nice to have a dedicated function for mapping whatever high-level permissions we decide to use to CHERI capability permissions.
This occurred to me, but the issue is exactly that there isn't a straightforward mapping and make_privileged_user_ptr() uses a different one, see the comments in both functions. I don't think there's much point in having two mapping functions that may not even be generic, I would delay that until there is another user for at least one of them.
Sound reasonable.
--- BR B
Kevin
- return cheri_check_cap_data_access(ptr, len, cap_perms);
+}
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