Hi,
I am posting this series now to gather some opinions (notably in terms of naming) before I proceed further.
The main focus is the introduction of two new user_ptr helpers: make_privileged_user_ptr() to create fine-grained user pointers (appropriate bounds and permissions), and check_user_ptr() to check user pointers. This does however require more involved CHERI operations than what we've used so far, and as a result it felt like the right time to introduce a new header with various CHERI-related definitions.
This new cheri.h header should be included in new code instead of the compiler-provided cheriintrin.h, notably because it is safe to include it unconditionally. linux/cheri.h is also a great place to introduce appropriate (CHERI-generic) root capabilities, which is another focus of this series. This makes it possible to have generic implementations of uaddr_to_user_ptr*() and get rid of asm/user_ptr.h.
The introduction of a root userspace capability with appropriate bounds and permissions is the only functional change from a userspace perspective: many capabilities given to userspace will now have bounds encompassing only the user address space and permissions corresponding to what is expected of an RWX capability in PCuABI. This work is to be continued by replacing most uses of morello_root_cap with cheri_root_cap_userspace (either in v2 or in a separate series).
On a similar theme, compat_ptr() should be modified to derive capabilities from the current user DDC, and the new seal/CID root capabilities should be used in binfmt_elf.c. This would complete the transition to appropriate root capabilities.
Back to the two new user_ptr helpers, make_privileged_user_ptr() is meant to replace uaddr_to_user_ptr_safe() and the latter should eventually disappear. This probably belongs to a different patch series, however the last patch provides an example of such a change. This work should probably wait until we start accessing user memory through capabilities in uaccess, as right now the capability metadata is not used anyway. Note that calls to uaddr_to_user_ptr() are workarounds in themselves and should all be eliminated eventually, so they are not considered here. Regarding check_user_ptr(), there is no immediate need for it - it will become relevant to implement explicit checking of user pointers (when get_user_pages() and friends are used).
Finally the user_ptr.rst documentation needs to be updated to reflect the new helpers, this is to be done in v2.
This series depends on Beata's handy printk patch for the warning messages. It was lightly tested and should be mostly fine, however note that compat_ptr() currently triggers warnings because it is implemented in terms of uaddr_to_user_ptr_safe() and compat_ptr() may be passed arbitrary integers. This will be fixed in v2 by appropriately deriving capabilities from DDC as mentioned above.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/cheri_ptr_api
Thanks, Kevin
Kevin Brodsky (9): linux/user_ptr.h: Remove kaddr_to_user_ptr() linux/user_ptr.h: Improve comment formatting arm64: uapi: Add asm/cheri.h linux/cheri.h: Introduce CHERI helpers arm64: morello: Implement cheri.h linux/user_ptr.h: Generic PCuABI impl for uaddr_to_user_ptr* arm64: Remove asm/user_ptr.h linux/user_ptr.h: Introduce fine-grained helpers mm/memory: Create fine-grained user pointer
Documentation/core-api/user_ptr.rst | 8 -- arch/Kconfig | 2 +- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/cheri.h | 14 ++++ arch/arm64/include/asm/user_ptr.h | 43 ---------- arch/arm64/include/uapi/asm/cheri.h | 7 ++ arch/arm64/kernel/morello.c | 39 +++++++-- include/linux/cheri.h | 122 ++++++++++++++++++++++++++++ include/linux/user_ptr.h | 113 +++++++++++++++++++------- lib/Makefile | 3 + lib/cheri.c | 67 +++++++++++++++ lib/user_ptr.c | 62 ++++++++++++++ mm/memory.c | 3 +- 13 files changed, 392 insertions(+), 93 deletions(-) create mode 100644 arch/arm64/include/asm/cheri.h delete mode 100644 arch/arm64/include/asm/user_ptr.h create mode 100644 arch/arm64/include/uapi/asm/cheri.h create mode 100644 include/linux/cheri.h create mode 100644 lib/cheri.c create mode 100644 lib/user_ptr.c
The removal of set_fs() is now firmly established and there should therefore be no need to create user pointers from kernel addresses any more. This means we can get rid of kaddr_to_user_ptr(), which is unused.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- Documentation/core-api/user_ptr.rst | 8 -------- arch/arm64/include/asm/user_ptr.h | 8 -------- include/linux/user_ptr.h | 16 ---------------- 3 files changed, 32 deletions(-)
diff --git a/Documentation/core-api/user_ptr.rst b/Documentation/core-api/user_ptr.rst index 21e02d4bd11b..925d22df2b9d 100644 --- a/Documentation/core-api/user_ptr.rst +++ b/Documentation/core-api/user_ptr.rst @@ -97,7 +97,6 @@ Each function covers a particular category of input integer:
- User-provided user address: ``uaddr_to_user_ptr()`` - Kernel-controlled user address: ``uaddr_to_user_ptr_safe()`` - - Kernel address: ``kaddr_to_user_ptr()``
* **Compat pointer**: ``compat_ptr()``
@@ -130,13 +129,6 @@ derived from in the PCuABI case. | | user address | mappings during | | kernel needs to access user memory using a bare | | | | process initialisation | | virtual address that is not provided by userspace. | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``kaddr_to_user_ptr()`` | Kernel address | [None currently] | Kernel root capability | There used to be a number of situations where kernel | -| | | | | memory was accessed through uaccess, requiring user | -| | | | | pointers to be created out of kernel addresses. | -| | | | | This should no longer be the case and this function | -| | | | | will be removed once it is confirmed that there is | -| | | | | no use-case left. | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ | ``compat_ptr()`` | Compat pointer | Pointer in a | Current user DDC | Must be used whenever converting a compat user | | | | user-provided | | pointer to a native user pointer. | | | | ``compat_*`` struct | | | diff --git a/arch/arm64/include/asm/user_ptr.h b/arch/arm64/include/asm/user_ptr.h index 745e7d34f25b..323ad0301cbd 100644 --- a/arch/arm64/include/asm/user_ptr.h +++ b/arch/arm64/include/asm/user_ptr.h @@ -30,14 +30,6 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) } #define uaddr_to_user_ptr_safe(addr) uaddr_to_user_ptr_safe(addr)
-static inline void __user *kaddr_to_user_ptr(ptraddr_t addr) -{ - uintcap_t root_cap = morello_get_root_cap(); - - return (void __user *)__builtin_cheri_address_set(root_cap, addr); -} -#define kaddr_to_user_ptr(addr) kaddr_to_user_ptr(addr) - #endif /* CONFIG_CHERI_PURECAP_UABI */
#endif /* __ASM_USER_PTR_H */ diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 0942b58cfb6a..e2de3464bcf8 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -65,22 +65,6 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) } #endif
-#ifndef kaddr_to_user_ptr -/** - * kaddr_to_user_ptr - convert a kernel address to a user pointer - * @addr: the address to set the pointer to - * - * Returns a user pointer with its address set to @addr. - * - * This function should be used when kernel memory needs to be accessed via a - * user pointer. There should be no use for it after the removal of set_fs(). - */ -static inline void __user *kaddr_to_user_ptr(ptraddr_t addr) -{ - return as_user_ptr(addr); -} -#endif - /** * user_ptr_addr - extract the address of a user pointer * @ptr: the user pointer to extract the address from
Format kernel-doc comments as per the recommendations in Documentation/doc-guide/kernel-doc.rst.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- include/linux/user_ptr.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index e2de3464bcf8..152eb7af6dde 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,8 +9,8 @@ #endif
/** - * as_user_ptr - convert an arbitrary integer value to a user pointer - * @x: the integer value to convert + * as_user_ptr() - Convert an arbitrary integer value to a user pointer. + * @x: The integer value to convert. * * Returns @x represented as a user pointer. The result is not a valid pointer * and shall not be dereferenced. @@ -30,8 +30,8 @@
#ifndef uaddr_to_user_ptr /** - * uaddr_to_user_ptr - convert a user-provided address to a user pointer - * @addr: the address to set the pointer to + * uaddr_to_user_ptr() - Convert a user-provided address to a user pointer. + * @addr: The address to set the pointer to. * * Returns a user pointer with its address set to @addr. * @@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr)
#ifndef uaddr_to_user_ptr_safe /** - * uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer - * @addr: the address to set the pointer to + * uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a + * user pointer. + * @addr: The address to set the pointer to. * * Returns a user pointer with its address set to @addr. * @@ -66,8 +67,8 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) #endif
/** - * user_ptr_addr - extract the address of a user pointer - * @ptr: the user pointer to extract the address from + * user_ptr_addr() - Extract the address of a user pointer. + * @ptr: The user pointer to extract the address from. * * Returns the address @ptr points to. */ @@ -77,7 +78,7 @@ static inline ptraddr_t user_ptr_addr(const void __user *ptr) }
/** - * user_ptr_is_same - checks where two user pointers are exactly the same + * user_ptr_is_same() - Checks where two user pointers are exactly the same. * * Returns true if @p1 and @p2 are exactly the same user pointers. *
So, out of curiosity, I did run: scripts/kernel-doc -v -none include/linux/user_ptr.h which did give some warnings on missing description for return values: the 'Returns xyz .....' should be 'Return: xyz ....'. That should not be a show stopper for this patch though. Also some of the comments could fall into 'Context' blocks ? Not sure though what is exact scope of intended changes here so feel free to ignore the comments here.
On Wed, Sep 28, 2022 at 04:31:41PM +0100, Kevin Brodsky wrote:
Format kernel-doc comments as per the recommendations in Documentation/doc-guide/kernel-doc.rst.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
include/linux/user_ptr.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index e2de3464bcf8..152eb7af6dde 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,8 +9,8 @@ #endif /**
- as_user_ptr - convert an arbitrary integer value to a user pointer
- @x: the integer value to convert
- as_user_ptr() - Convert an arbitrary integer value to a user pointer.
- @x: The integer value to convert.
- Returns @x represented as a user pointer. The result is not a valid pointer
- and shall not be dereferenced.
@@ -30,8 +30,8 @@ #ifndef uaddr_to_user_ptr /**
- uaddr_to_user_ptr - convert a user-provided address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr() - Convert a user-provided address to a user pointer.
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) #ifndef uaddr_to_user_ptr_safe /**
- uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
- user pointer.
This one seems bit weird. The guidelines do mention indentation for mutli-line argument description, not for function brief description though. So I guess we could either follow the arguments formatting or get rid of this indent whatsoever maybe ?
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -66,8 +67,8 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) #endif /**
- user_ptr_addr - extract the address of a user pointer
- @ptr: the user pointer to extract the address from
- user_ptr_addr() - Extract the address of a user pointer.
*/
- @ptr: The user pointer to extract the address from.
- Returns the address @ptr points to.
@@ -77,7 +78,7 @@ static inline ptraddr_t user_ptr_addr(const void __user *ptr) } /**
- user_ptr_is_same - checks where two user pointers are exactly the same
- user_ptr_is_same() - Checks where two user pointers are exactly the same.
scripts/kernel-doc does complain about missing arguments' description.
--- BR B.
- Returns true if @p1 and @p2 are exactly the same user pointers.
-- 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
On 11/10/2022 21:59, Beata Michalska wrote:
So, out of curiosity, I did run: scripts/kernel-doc -v -none include/linux/user_ptr.h
I didn't even know about that functionality so thanks!
which did give some warnings on missing description for return values: the 'Returns xyz .....' should be 'Return: xyz ....'.
I hadn't realised these were required in principle, I'll add them. In most cases I'll just convert the main block into a "Return:" block with indentation as there's nothing else to say really.
That should not be a show stopper for this patch though. Also some of the comments could fall into 'Context' blocks ?
I think not really, as this is "Context" in a very specific kernel sense, as per the documentation:
* * Context: Describes whether the function can sleep, what locks it takes, * releases, or expects to be held. It can extend over multiple * lines.
Not sure though what is exact scope of intended changes here so feel free to ignore the comments here.
Might as well make kernel-doc happy, this way we have a clear guideline for the future. It does complain about not understanding __capability in cheri.h but we can ignore that.
On Wed, Sep 28, 2022 at 04:31:41PM +0100, Kevin Brodsky wrote:
Format kernel-doc comments as per the recommendations in Documentation/doc-guide/kernel-doc.rst.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
include/linux/user_ptr.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index e2de3464bcf8..152eb7af6dde 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,8 +9,8 @@ #endif /**
- as_user_ptr - convert an arbitrary integer value to a user pointer
- @x: the integer value to convert
- as_user_ptr() - Convert an arbitrary integer value to a user pointer.
- @x: The integer value to convert.
- Returns @x represented as a user pointer. The result is not a valid pointer
- and shall not be dereferenced.
@@ -30,8 +30,8 @@ #ifndef uaddr_to_user_ptr /**
- uaddr_to_user_ptr - convert a user-provided address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr() - Convert a user-provided address to a user pointer.
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) #ifndef uaddr_to_user_ptr_safe /**
- uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
- user pointer.
This one seems bit weird. The guidelines do mention indentation for mutli-line argument description, not for function brief description though. So I guess we could either follow the arguments formatting or get rid of this indent whatsoever maybe ?
I wanted feedback on this one actually as it is indeed not specified, but I kind of felt that some indentation made it easier to read. If you feel it's better without I can easily remove it.
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -66,8 +67,8 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) #endif /**
- user_ptr_addr - extract the address of a user pointer
- @ptr: the user pointer to extract the address from
- user_ptr_addr() - Extract the address of a user pointer.
*/
- @ptr: The user pointer to extract the address from.
- Returns the address @ptr points to.
@@ -77,7 +78,7 @@ static inline ptraddr_t user_ptr_addr(const void __user *ptr) } /**
- user_ptr_is_same - checks where two user pointers are exactly the same
- user_ptr_is_same() - Checks where two user pointers are exactly the same.
scripts/kernel-doc does complain about missing arguments' description.
Will add too (just to make it happy as it's really quite self-descriptive :) ).
Kevin
BR B.
- Returns true if @p1 and @p2 are exactly the same user pointers.
-- 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
On Wed, Oct 12, 2022 at 07:04:38PM +0200, Kevin Brodsky wrote:
On 11/10/2022 21:59, Beata Michalska wrote:
So, out of curiosity, I did run: scripts/kernel-doc -v -none include/linux/user_ptr.h
I didn't even know about that functionality so thanks!
which did give some warnings on missing description for return values: the 'Returns xyz .....' should be 'Return: xyz ....'.
I hadn't realised these were required in principle, I'll add them. In most cases I'll just convert the main block into a "Return:" block with indentation as there's nothing else to say really.
That should not be a show stopper for this patch though. Also some of the comments could fall into 'Context' blocks ?
I think not really, as this is "Context" in a very specific kernel sense, as per the documentation:
* * Context: Describes whether the function can sleep, what locks it takes, * releases, or expects to be held. It can extend over multiple * lines.
I did a quick scan through the code, and indeed it seems it is not applicable in this case.
Not sure though what is exact scope of intended changes here so feel free to ignore the comments here.
Might as well make kernel-doc happy, this way we have a clear guideline for the future. It does complain about not understanding __capability in cheri.h but we can ignore that.
On Wed, Sep 28, 2022 at 04:31:41PM +0100, Kevin Brodsky wrote:
Format kernel-doc comments as per the recommendations in Documentation/doc-guide/kernel-doc.rst.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
include/linux/user_ptr.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index e2de3464bcf8..152eb7af6dde 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,8 +9,8 @@ #endif /**
- as_user_ptr - convert an arbitrary integer value to a user pointer
- @x: the integer value to convert
- as_user_ptr() - Convert an arbitrary integer value to a user pointer.
- @x: The integer value to convert.
- Returns @x represented as a user pointer. The result is not a valid pointer
- and shall not be dereferenced.
@@ -30,8 +30,8 @@ #ifndef uaddr_to_user_ptr /**
- uaddr_to_user_ptr - convert a user-provided address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr() - Convert a user-provided address to a user pointer.
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) #ifndef uaddr_to_user_ptr_safe /**
- uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
- user pointer.
This one seems bit weird. The guidelines do mention indentation for mutli-line argument description, not for function brief description though. So I guess we could either follow the arguments formatting or get rid of this indent whatsoever maybe ?
I wanted feedback on this one actually as it is indeed not specified, but I kind of felt that some indentation made it easier to read. If you feel it's better without I can easily remove it.
I was actually thinking of having the indentation aligned with the start of the description itself, so same as for arguments:
" If the ``@argument`` description has multiple lines, the continuation of the description should start at the same column as the previous line::"
but no strong opinion here.
--- BR B.
- @addr: The address to set the pointer to.
- Returns a user pointer with its address set to @addr.
@@ -66,8 +67,8 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) #endif /**
- user_ptr_addr - extract the address of a user pointer
- @ptr: the user pointer to extract the address from
- user_ptr_addr() - Extract the address of a user pointer.
*/
- @ptr: The user pointer to extract the address from.
- Returns the address @ptr points to.
@@ -77,7 +78,7 @@ static inline ptraddr_t user_ptr_addr(const void __user *ptr) } /**
- user_ptr_is_same - checks where two user pointers are exactly the same
- user_ptr_is_same() - Checks where two user pointers are exactly the same.
scripts/kernel-doc does complain about missing arguments' description.
Will add too (just to make it happy as it's really quite self-descriptive :) ).
Kevin
BR B.
- Returns true if @p1 and @p2 are exactly the same user pointers.
-- 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
On 18/10/2022 22:21, Beata Michalska wrote:
@@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) #ifndef uaddr_to_user_ptr_safe /**
- uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
- user pointer.
This one seems bit weird. The guidelines do mention indentation for mutli-line argument description, not for function brief description though. So I guess we could either follow the arguments formatting or get rid of this indent whatsoever maybe ?
I wanted feedback on this one actually as it is indeed not specified, but I kind of felt that some indentation made it easier to read. If you feel it's better without I can easily remove it.
I was actually thinking of having the indentation aligned with the start of the description itself, so same as for arguments:
" If the ``@argument`` description has multiple lines, the continuation of the description should start at the same column as the previous line::"
but no strong opinion here.
Right so to be clear do you mean the column where the description starts after the "-"? That is:
+ * uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a + * user pointer.
I think that would make sense indeed.
Kevin
On Fri, Oct 21, 2022 at 01:24:09PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:21, Beata Michalska wrote:
@@ -50,8 +50,9 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) #ifndef uaddr_to_user_ptr_safe /**
- uaddr_to_user_ptr_safe - convert a kernel-generated user address to a user pointer
- @addr: the address to set the pointer to
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
- user pointer.
This one seems bit weird. The guidelines do mention indentation for mutli-line argument description, not for function brief description though. So I guess we could either follow the arguments formatting or get rid of this indent whatsoever maybe ?
I wanted feedback on this one actually as it is indeed not specified, but I kind of felt that some indentation made it easier to read. If you feel it's better without I can easily remove it.
I was actually thinking of having the indentation aligned with the start of the description itself, so same as for arguments:
" If the ``@argument`` description has multiple lines, the continuation of the description should start at the same column as the previous line::"
but no strong opinion here.
Right so to be clear do you mean the column where the description starts after the "-"? That is:
- uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a
user pointer.I think that would make sense indeed.
Yes, that's what I had in mind. But then again, that was just a suggestion. --- B.
Kevin
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H + +#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */ + +#endif /* _UAPI__ASM_CHERI_H */
Hi,
On 9/28/22 21:01, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
Can this be placed in uapi/linux/cheri.h ? May be it will help non arch mmap code to use it directly.
Thanks, Amit
+#endif /* _UAPI__ASM_CHERI_H */
On 07/10/2022 15:25, Amit Kachhap wrote:
Hi,
On 9/28/22 21:01, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
Can this be placed in uapi/linux/cheri.h ? May be it will help non arch mmap code to use it directly.
Unfortunately not. Although it should be possible to use the first SW permission bit for VMem on any CHERI architecture, this value is Morello-specific as the permission encoding is specific to each architecture.
Kevin
Thanks, Amit
+#endif /* _UAPI__ASM_CHERI_H */
On Wed, Sep 28, 2022 at 04:31:42PM +0100, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
This is Morello specific software permission bit, in somewhat generic header, but I do appreciate it's arch specific so there should be no confusion there, still it made be ... pause for a moment there. Not sure if adding a comment would make much difference here though.
--- BR B.
+#endif /* _UAPI__ASM_CHERI_H */
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
On 11/10/2022 22:05, Beata Michalska wrote:
On Wed, Sep 28, 2022 at 04:31:42PM +0100, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
This is Morello specific software permission bit, in somewhat generic header, but I do appreciate it's arch specific so there should be no confusion there, still it made be ... pause for a moment there. Not sure if adding a comment would make much difference here though.
It is a Morello-specific value for a CHERI-generic constant (each CHERI architecture needs to define the appropriate value in its own uapi/asm/cheri.h). I'm not sure what exactly to say here as it's simply the implementation of the spec.
Kevin
BR B.
+#endif /* _UAPI__ASM_CHERI_H */
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
On Wed, Oct 12, 2022 at 07:08:48PM +0200, Kevin Brodsky wrote:
On 11/10/2022 22:05, Beata Michalska wrote:
On Wed, Sep 28, 2022 at 04:31:42PM +0100, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
This is Morello specific software permission bit, in somewhat generic header, but I do appreciate it's arch specific so there should be no confusion there, still it made be ... pause for a moment there. Not sure if adding a comment would make much difference here though.
It is a Morello-specific value for a CHERI-generic constant (each CHERI architecture needs to define the appropriate value in its own uapi/asm/cheri.h). I'm not sure what exactly to say here as it's simply the implementation of the spec.
I guess I was looking for any mention of Morello here (may it be a comment). But overall you are right and it is supposed to be self-explanatory, so feel free to ignore me here.
--- BR B.
Kevin
BR B.
+#endif /* _UAPI__ASM_CHERI_H */
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
On 18/10/2022 22:15, Beata Michalska wrote:
On Wed, Oct 12, 2022 at 07:08:48PM +0200, Kevin Brodsky wrote:
On 11/10/2022 22:05, Beata Michalska wrote:
On Wed, Sep 28, 2022 at 04:31:42PM +0100, Kevin Brodsky wrote:
Add a new uapi header to define generic CHERI concepts with arm64-specific values.
To start with, provide a macro representing the VMem software permission, as defined in the PCuABI specification. The macro name matches CheriBSD, facilitating its use in a (somewhat) OS-agnostic way.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/include/uapi/asm/cheri.h | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 arch/arm64/include/uapi/asm/cheri.h
diff --git a/arch/arm64/include/uapi/asm/cheri.h b/arch/arm64/include/uapi/asm/cheri.h new file mode 100644 index 000000000000..8f2b8f885fda --- /dev/null +++ b/arch/arm64/include/uapi/asm/cheri.h @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__ASM_CHERI_H +#define _UAPI__ASM_CHERI_H
+#define CHERI_PERM_SW_VMEM (1 << 2) /* User[0] permission */
This is Morello specific software permission bit, in somewhat generic header, but I do appreciate it's arch specific so there should be no confusion there, still it made be ... pause for a moment there. Not sure if adding a comment would make much difference here though.
It is a Morello-specific value for a CHERI-generic constant (each CHERI architecture needs to define the appropriate value in its own uapi/asm/cheri.h). I'm not sure what exactly to say here as it's simply the implementation of the spec.
I guess I was looking for any mention of Morello here (may it be a comment).
Right I hadn't even realised there was no mention of Morello either in the filename or the constant name, I see how it can be confusing now :) Will expand the comment to say what User[0] actually means (i.e. the Morello permission encoding).
Kevin
But overall you are right and it is supposed to be self-explanatory, so feel free to ignore me here.
BR B.
Kevin
BR B.
+#endif /* _UAPI__ASM_CHERI_H */
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
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 + 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 + +/** + * 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 + * 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: + * 1. @addr is a valid userspace address. + * 2. The (@addr, @len) tuple can be represented as capability bounds. + * 3. @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: + * 1. @addr is a valid userspace address. + * 2. @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); + + +/** + * 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); + + +/* + * 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; + +#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__ */
On 9/28/22 21:01, 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
- 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)
Should this look better with CHERI_PERMS_READ anded as well ?
+#endif
+#ifndef CHERI_PERMS_ROOTCAP +#define CHERI_PERMS_ROOTCAP \
- (CHERI_PERM_GLOBAL | CHERI_PERM_SW_VMEM)
+#endif
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
Thanks, Amit
+/**
- 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);+/**
- 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);+/*
- 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;
+#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__ */
On 07/10/2022 15:27, Amit Kachhap wrote:
On 9/28/22 21:01, 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
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)
Should this look better with CHERI_PERMS_READ anded as well ?
I prefer to be explicit here, although you are right that in practice this will always be and'ed with READ. Note that these permission sets correspond to the *_CAP_PERMS sets in the spec.
+#endif
+#ifndef CHERI_PERMS_ROOTCAP +#define CHERI_PERMS_ROOTCAP \ + (CHERI_PERM_GLOBAL | CHERI_PERM_SW_VMEM) +#endif
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
Kevin
Thanks, Amit
+/**
- 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);
+/**
- 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);
+/*
- 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;
+#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__ */
On Thu, Oct 13, 2022 at 10:17:23AM +0200, Kevin Brodsky wrote:
On 07/10/2022 15:27, Amit Kachhap wrote:
On 9/28/22 21:01, 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
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)
Should this look better with CHERI_PERMS_READ anded as well ?
I prefer to be explicit here, although you are right that in practice this will always be and'ed with READ. Note that these permission sets correspond to the *_CAP_PERMS sets in the spec.
+#endif
+#ifndef CHERI_PERMS_ROOTCAP +#define CHERI_PERMS_ROOTCAP \ + (CHERI_PERM_GLOBAL | CHERI_PERM_SW_VMEM) +#endif
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though... (cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess). On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
--- BR B.
Kevin
Thanks, Amit
+/**
- 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);
+/**
- 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);
+/*
- 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;
+#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__ */
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 18/10/2022 22:20, Beata Michalska wrote:
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though...
I suppose we could indeed add an argument later to ask for a sentry (RB-sealed capability on Morello).
(cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess).
We are specifically talking about sentries here (RB object type on Morello). It is expected that all function pointers are sentries in purecap.
On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
I would really like this to be explicit, but like the build function it could be a new argument.
So to summarise my thinking after your feedback (thanks for that!): * cheri_build_user_cap() remains unchanged. * cheri_check_cap_data_access() can be simply cheri_check_cap(), the meaning of "check" being inferred from the arguments passed to the function.
"data" is gone, if and when we want to handle things like function pointers we can add a new argument or create special functions.
Does that sound fair?
Kevin
On Fri, Oct 21, 2022 at 02:33:51PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:20, Beata Michalska wrote:
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though...
I suppose we could indeed add an argument later to ask for a sentry (RB-sealed capability on Morello).
(cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess).
We are specifically talking about sentries here (RB object type on Morello). It is expected that all function pointers are sentries in purecap.
Right, though that does not work the other way round: sentry (RB type sealed capability) is just one of the flavours of sealed capabilities. And even though for the cheri_check_cap_data_access, sealed capability is a no-go, as it is undereferenceable and we are checking for access permissions explicitly here, it is not so much obvious for building a capability: I guess we should be able to create sealed data capability ? (cannot think of a use case now though ... )
On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
I would really like this to be explicit, but like the build function it could be a new argument.
I do understand the drive for being 'explicit' here, especially the longer I let it sink in.
So to summarise my thinking after your feedback (thanks for that!):
- cheri_build_user_cap() remains unchanged.
- cheri_check_cap_data_access() can be simply cheri_check_cap(), the meaning
of "check" being inferred from the arguments passed to the function.
"data" is gone, if and when we want to handle things like function pointers we can add a new argument or create special functions.
Does that sound fair?
It does indeed.
Small note: compiler's CHERI intrinsics use cheri_cap_<action> notation, so we could go for that as well (cheri_cap_check) ? Just an idea, do not mind either.
--- BR. B
Kevin
On 25/10/2022 16:02, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:33:51PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:20, Beata Michalska wrote:
+/**
- 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
- 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);
This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though...
I suppose we could indeed add an argument later to ask for a sentry (RB-sealed capability on Morello).
(cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess).
We are specifically talking about sentries here (RB object type on Morello). It is expected that all function pointers are sentries in purecap.
Right, though that does not work the other way round: sentry (RB type sealed capability) is just one of the flavours of sealed capabilities. And even though for the cheri_check_cap_data_access, sealed capability is a no-go, as it is undereferenceable and we are checking for access permissions explicitly here,
That's indeed what I meant regarding sentries.
it is not so much obvious for building a capability: I guess we should be able to create sealed data capability ? (cannot think of a use case now though ... )
Possibly one day, though there is no such case in PCuABI as things stand. In any case I certain don't think that cheri.h as introduced by this patch is complete, it's simply a starting point with functionalities that will definitely be needed :)
On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
I would really like this to be explicit, but like the build function it could be a new argument.
I do understand the drive for being 'explicit' here, especially the longer I let it sink in.
So to summarise my thinking after your feedback (thanks for that!):
- cheri_build_user_cap() remains unchanged.
- cheri_check_cap_data_access() can be simply cheri_check_cap(), the meaning
of "check" being inferred from the arguments passed to the function.
"data" is gone, if and when we want to handle things like function pointers we can add a new argument or create special functions.
Does that sound fair?
It does indeed.
Small note: compiler's CHERI intrinsics use cheri_cap_<action> notation, so we could go for that as well (cheri_cap_check) ? Just an idea, do not mind either.
That's a very valid point. However I'm rather unsure about "cheri_cap_build_user()", because "user" is about "cap" and not "build". Also note that "cap" is only present in "cheri_cap_build()", other functions are typically called "cheri_<attr>_{get,set}".
This also makes me wonder if using the same "cheri_" prefix as cheriintrin.h might not get confusing. On the other hand, I'm thinking of <linux/cheri.h> as the "new <cheriintrin.h>" for general use in the kernel, so cheriintrin.h could be seen as just an implementation detail for part of the "cheri_*" interface.
I'm on the fence on both of these angles, right now I would keep the names I proposed above unchanged but more opinions are welcome!
Kevin
BR. B
Kevin
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 01/11/2022 09:51, Kevin Brodsky wrote:
On 25/10/2022 16:02, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:33:51PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:20, Beata Michalska wrote:
> +/** > + * 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 > + * 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: > + * 1. @addr is a valid userspace address. > + * 2. The (@addr, @len) tuple can be represented as capability > bounds. > + * 3. @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); This builds full capability data so may be cheri_build_user_cap_data( used below for access function) or cheri_build_user_data.
I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though...
I suppose we could indeed add an argument later to ask for a sentry (RB-sealed capability on Morello).
(cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess).
We are specifically talking about sentries here (RB object type on Morello). It is expected that all function pointers are sentries in purecap.
Right, though that does not work the other way round: sentry (RB type sealed capability) is just one of the flavours of sealed capabilities. And even though for the cheri_check_cap_data_access, sealed capability is a no-go, as it is undereferenceable and we are checking for access permissions explicitly here,
That's indeed what I meant regarding sentries.
it is not so much obvious for building a capability: I guess we should be able to create sealed data capability ? (cannot think of a use case now though ... )
Possibly one day, though there is no such case in PCuABI as things stand. In any case I certain don't think that cheri.h as introduced by this patch is complete, it's simply a starting point with functionalities that will definitely be needed :)
On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
I would really like this to be explicit, but like the build function it could be a new argument.
I do understand the drive for being 'explicit' here, especially the longer I let it sink in.
So to summarise my thinking after your feedback (thanks for that!):
- cheri_build_user_cap() remains unchanged.
- cheri_check_cap_data_access() can be simply cheri_check_cap(), the
meaning of "check" being inferred from the arguments passed to the function.
No comments other than this seems reasonable given the above thread.
"data" is gone, if and when we want to handle things like function pointers we can add a new argument or create special functions.
Does that sound fair?
It does indeed.
Small note: compiler's CHERI intrinsics use cheri_cap_<action> notation, so we could go for that as well (cheri_cap_check) ? Just an idea, do not mind either.
That's a very valid point. However I'm rather unsure about "cheri_cap_build_user()", because "user" is about "cap" and not "build". Also note that "cap" is only present in "cheri_cap_build()", other functions are typically called "cheri_<attr>_{get,set}".
This also makes me wonder if using the same "cheri_" prefix as cheriintrin.h might not get confusing. On the other hand, I'm thinking of <linux/cheri.h> as the "new <cheriintrin.h>" for general use in the kernel, so cheriintrin.h could be seen as just an implementation detail for part of the "cheri_*" interface.
I'm on the fence on both of these angles, right now I would keep the names I proposed above unchanged but more opinions are welcome!
Agreed that cheri_build_user_cap() is clearer.
Thanks Zach
Kevin
BR. B
Kevin
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
On 02/11/2022 17:59, Zachary Leaf wrote:
On 01/11/2022 09:51, Kevin Brodsky wrote:
On 25/10/2022 16:02, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:33:51PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:20, Beata Michalska wrote:
>> +/** >> + * 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 >> + * 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: >> + * 1. @addr is a valid userspace address. >> + * 2. The (@addr, @len) tuple can be represented as capability >> bounds. >> + * 3. @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); > This builds full capability data so may be cheri_build_user_cap_data( > used below for access function) or cheri_build_user_data. I see your point. It all boils down to sealing in fact, as everything else is specified to the function as parameters. For cheri_check_cap_data_access() I thought it was important to say "data" as you cannot use this function to check a function pointer - it would be RB-sealed and therefore fail the sealing test.
I was less sure it made sense to say "data" for this function as it can be used to construct capabilities with any permissions, including Executable, but arguably it's also true that you can check for Executable using cheri_check_cap_data_access(). We should probably say "data" in both or neither.
I guess this choice is related to another, which is what we would do if we had to handle function pointers (I don't expect we will, but it helps thinking about it). There are essentially two options: 1. have a new set of functions with "code" in their name, or 2. use the same functions and do the sealing/unsealing manually. 1. corresponds to saying "data" for the present functions, and 2. to not saying "data".
Maybe we should keep it simple and go for 2. but opinions welcome here.
I am not entirely convinced that having the distinction here 'data' vs 'code' would be very much useful: this function could create both, and it can be extended to cover sealing case needed - not sure why manual sealing though...
I suppose we could indeed add an argument later to ask for a sentry (RB-sealed capability on Morello).
(cheri_root_seal_cap_userspace is already/will be there ?) (on a side note, sealed capability does not automatically denote function pointers even though that is not the use case we will be supporting, I guess).
We are specifically talking about sentries here (RB object type on Morello). It is expected that all function pointers are sentries in purecap.
Right, though that does not work the other way round: sentry (RB type sealed capability) is just one of the flavours of sealed capabilities. And even though for the cheri_check_cap_data_access, sealed capability is a no-go, as it is undereferenceable and we are checking for access permissions explicitly here,
That's indeed what I meant regarding sentries.
it is not so much obvious for building a capability: I guess we should be able to create sealed data capability ? (cannot think of a use case now though ... )
Possibly one day, though there is no such case in PCuABI as things stand. In any case I certain don't think that cheri.h as introduced by this patch is complete, it's simply a starting point with functionalities that will definitely be needed :)
On the other hand chari_check_cap_data_access could potentially check whether the cap has Executable permission set if selaed and consider that as valid ?
I would really like this to be explicit, but like the build function it could be a new argument.
I do understand the drive for being 'explicit' here, especially the longer I let it sink in.
So to summarise my thinking after your feedback (thanks for that!):
- cheri_build_user_cap() remains unchanged.
- cheri_check_cap_data_access() can be simply cheri_check_cap(), the
meaning of "check" being inferred from the arguments passed to the function.
No comments other than this seems reasonable given the above thread.
"data" is gone, if and when we want to handle things like function pointers we can add a new argument or create special functions.
Does that sound fair?
It does indeed.
Small note: compiler's CHERI intrinsics use cheri_cap_<action> notation, so we could go for that as well (cheri_cap_check) ? Just an idea, do not mind either.
That's a very valid point. However I'm rather unsure about "cheri_cap_build_user()", because "user" is about "cap" and not "build". Also note that "cap" is only present in "cheri_cap_build()", other functions are typically called "cheri_<attr>_{get,set}".
This also makes me wonder if using the same "cheri_" prefix as cheriintrin.h might not get confusing. On the other hand, I'm thinking of <linux/cheri.h> as the "new <cheriintrin.h>" for general use in the kernel, so cheriintrin.h could be seen as just an implementation detail for part of the "cheri_*" interface.
I'm on the fence on both of these angles, right now I would keep the names I proposed above unchanged but more opinions are welcome!
Agreed that cheri_build_user_cap() is clearer.
Thanks for the feedback! Will go for that then.
Kevin
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 ?
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 ?
+/**
- 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.
- 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) ?
+/**
- 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 ?
+/*
- 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. Also, it took me a moment to translate cid to compartment ID :)
--- 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
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.
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.
+/**
- 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.
+/**
- 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).
+/*
- 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?
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!
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
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.
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. 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.
+/**
- 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.
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. 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
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
On Fri, Oct 21, 2022 at 02:34:14PM +0200, Kevin Brodsky wrote:
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.
Right, missed some bits here. Makes sense now - thanks for clarifying and for patience!
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 :)
Too much indeed.
--- B.
+/**
- 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
Implement the Morello-specific aspects of linux/cheri.h, that is:
* Override standard permission sets to include the appropriate Morello-specific permissions.
* Initialise the global root capabilities appropriately.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/cheri.h | 14 ++++++++++++ arch/arm64/kernel/morello.c | 39 +++++++++++++++++++++++++++------- 3 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 arch/arm64/include/asm/cheri.h
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index d402721ccad0..a0635a459a67 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -29,6 +29,7 @@ config ARM64 select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_GIGANTIC_PAGE select ARCH_HAS_KCOV + select ARCH_HAS_CHERI_H select ARCH_HAS_USER_PTR_H select ARCH_HAS_KEEPINITRD select ARCH_HAS_MEMBARRIER_SYNC_CORE diff --git a/arch/arm64/include/asm/cheri.h b/arch/arm64/include/asm/cheri.h new file mode 100644 index 000000000000..c35c17cb4b6a --- /dev/null +++ b/arch/arm64/include/asm/cheri.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __ASM_CHERI_H +#define __ASM_CHERI_H + +#define CHERI_PERMS_READ \ + (CHERI_PERM_LOAD | CHERI_PERM_LOAD_CAP | ARM_CAP_PERMISSION_MUTABLE_LOAD) + +#define CHERI_PERMS_EXEC \ + (CHERI_PERM_EXECUTE | CHERI_PERM_SYSTEM_REGS | ARM_CAP_PERMISSION_EXECUTIVE) + +#define CHERI_PERMS_ROOTCAP \ + (CHERI_PERM_GLOBAL | CHERI_PERM_SW_VMEM | ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR) + +#endif /* __ASM_CHERI_H */ diff --git a/arch/arm64/kernel/morello.c b/arch/arm64/kernel/morello.c index ccbf26e77919..a9f2c0f1e56e 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> @@ -307,17 +306,41 @@ 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, 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); + ctemp = cheri_perms_and(ctemp, CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | + CHERI_PERMS_WRITE | CHERI_PERMS_EXEC); + cheri_root_cap_userspace = ctemp; + + ctemp = cheri_address_set(root_cap, 0); + /* + * 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_root_seal_cap_userspace = ctemp;
- morello_root_cap = (uintcap_t)cheri_ddc_get(); + /* Maximum bounds for the time being. */ + ctemp = root_cap; + ctemp = cheri_perms_and(ctemp, CHERI_PERM_GLOBAL | + ARM_CAP_PERMISSION_COMPARTMENT_ID); + cheri_root_cid_cap_userspace = 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;
Now that linux/cheri.h provides CHERI-generic functionalities, it is possible to implement uaddr_to_user_ptr* for PCuABI in an arch-agnostic way.
With this patch, the PCuABI version is implemented out of line in lib/user_ptr.c. This is notably to avoid pulling in linux/cheri.h in linux/user_ptr.h, itself included from linux/kernel.h.
This approach supersedes asm/user_ptr.h, which is no longer used and will be subsequently removed.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- include/linux/user_ptr.h | 26 ++++++++++++++------------ lib/Makefile | 1 + lib/user_ptr.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 lib/user_ptr.c
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 152eb7af6dde..21cc0f6e1832 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -4,10 +4,6 @@
#include <linux/typecheck.h>
-#ifdef CONFIG_ARCH_HAS_USER_PTR_H -#include <asm/user_ptr.h> -#endif - /** * as_user_ptr() - Convert an arbitrary integer value to a user pointer. * @x: The integer value to convert. @@ -28,7 +24,8 @@ /* Legacy user pointer conversion macro, new code should use as_user_ptr() */ #define u64_to_user_ptr(x) as_user_ptr_strict(u64, (x))
-#ifndef uaddr_to_user_ptr +#ifdef CONFIG_CHERI_PURECAP_UABI + /** * uaddr_to_user_ptr() - Convert a user-provided address to a user pointer. * @addr: The address to set the pointer to. @@ -42,13 +39,8 @@ * When the pure-capability uABI is targeted, uses of this function bypass the * capability model and should be minimised. */ -static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) -{ - return as_user_ptr(addr); -} -#endif +void __user *uaddr_to_user_ptr(ptraddr_t addr);
-#ifndef uaddr_to_user_ptr_safe /** * uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a * user pointer. @@ -60,11 +52,21 @@ static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) * 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). */ +void __user *uaddr_to_user_ptr_safe(ptraddr_t addr); + +#else /* CONFIG_CHERI_PURECAP_UABI */ + +static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) +{ + return as_user_ptr(addr); +} + static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) { return as_user_ptr(addr); } -#endif + +#endif /* CONFIG_CHERI_PURECAP_UABI */
/** * user_ptr_addr() - Extract the address of a user pointer. diff --git a/lib/Makefile b/lib/Makefile index 8dd8c00fee31..8bdd488e7e59 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -264,6 +264,7 @@ obj-$(CONFIG_STMP_DEVICE) += stmp_device.o obj-$(CONFIG_IRQ_POLL) += irq_poll.o
obj-y += cheri.o +obj-$(CONFIG_CHERI_PURECAP_UABI) += user_ptr.o
# stackdepot.c should not be instrumented or call instrumented functions. # Prevent the compiler from calling builtins like memcmp() or bcmp() from this diff --git a/lib/user_ptr.c b/lib/user_ptr.c new file mode 100644 index 000000000000..503a8369d019 --- /dev/null +++ b/lib/user_ptr.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include <linux/bug.h> +#include <linux/cheri.h> +#include <linux/user_ptr.h> + +void __user *uaddr_to_user_ptr(ptraddr_t addr) +{ + uintcap_t root_cap = cheri_root_cap_userspace; + + /* + * No warning if the result is invalid as the input address is not + * controlled by the kernel. + */ + return (void __user *)cheri_address_set(root_cap, addr); +} + +void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) +{ + uintcap_t root_cap = cheri_root_cap_userspace; + void __user *ret; + + ret = (void __user *)cheri_address_set(root_cap, addr); + + WARN(!cheri_tag_get(ret), + "Invalid user capability created: %#lp\n", ret); + + return ret; +} +
linux/user_ptr.h now uses generic PCuABI implementations of uaddr_to_user_ptr* and no longer includes asm/user_ptr.h. Time to remove it along with the corresponding config option.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/Kconfig | 3 --- arch/arm64/Kconfig | 1 - arch/arm64/include/asm/user_ptr.h | 35 ------------------------------- 3 files changed, 39 deletions(-) delete mode 100644 arch/arm64/include/asm/user_ptr.h
diff --git a/arch/Kconfig b/arch/Kconfig index b173a07c5a91..db0f652580c6 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1379,9 +1379,6 @@ config HAVE_ARCH_NODE_DEV_GROUP config ARCH_HAS_CHERI_H bool
-config ARCH_HAS_USER_PTR_H - bool - config ARCH_HAS_CHERI_CAPABILITIES bool
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a0635a459a67..5bc5ab5e8689 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -30,7 +30,6 @@ config ARM64 select ARCH_HAS_GIGANTIC_PAGE select ARCH_HAS_KCOV select ARCH_HAS_CHERI_H - select ARCH_HAS_USER_PTR_H select ARCH_HAS_KEEPINITRD select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE diff --git a/arch/arm64/include/asm/user_ptr.h b/arch/arm64/include/asm/user_ptr.h deleted file mode 100644 index 323ad0301cbd..000000000000 --- a/arch/arm64/include/asm/user_ptr.h +++ /dev/null @@ -1,35 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -#ifndef __ASM_USER_PTR_H -#define __ASM_USER_PTR_H - -#include <asm/morello.h> - -#ifdef CONFIG_CHERI_PURECAP_UABI - -static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) -{ - /* - * TODO [PCuABI] - the user root capability should be used, not the - * kernel one. - */ - uintcap_t root_cap = morello_get_root_cap(); - - return (void __user *)__builtin_cheri_address_set(root_cap, addr); -} -#define uaddr_to_user_ptr(addr) uaddr_to_user_ptr(addr) - -static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) -{ - /* - * TODO [PCuABI] - the user root capability should be used, not the - * kernel one. - */ - uintcap_t root_cap = morello_get_root_cap(); - - return (void __user *)__builtin_cheri_address_set(root_cap, addr); -} -#define uaddr_to_user_ptr_safe(addr) uaddr_to_user_ptr_safe(addr) - -#endif /* CONFIG_CHERI_PURECAP_UABI */ - -#endif /* __ASM_USER_PTR_H */
On 9/28/22 21:01, Kevin Brodsky wrote:
linux/user_ptr.h now uses generic PCuABI implementations of uaddr_to_user_ptr* and no longer includes asm/user_ptr.h. Time to remove it along with the corresponding config option.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/Kconfig | 3 --- arch/arm64/Kconfig | 1 - arch/arm64/include/asm/user_ptr.h | 35 ------------------------------- 3 files changed, 39 deletions(-) delete mode 100644 arch/arm64/include/asm/user_ptr.h
diff --git a/arch/Kconfig b/arch/Kconfig index b173a07c5a91..db0f652580c6 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1379,9 +1379,6 @@ config HAVE_ARCH_NODE_DEV_GROUP config ARCH_HAS_CHERI_H bool -config ARCH_HAS_USER_PTR_H
- bool
- config ARCH_HAS_CHERI_CAPABILITIES bool
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a0635a459a67..5bc5ab5e8689 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -30,7 +30,6 @@ config ARM64 select ARCH_HAS_GIGANTIC_PAGE select ARCH_HAS_KCOV select ARCH_HAS_CHERI_H
- select ARCH_HAS_USER_PTR_H select ARCH_HAS_KEEPINITRD select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
diff --git a/arch/arm64/include/asm/user_ptr.h b/arch/arm64/include/asm/user_ptr.h deleted file mode 100644 index 323ad0301cbd..000000000000 --- a/arch/arm64/include/asm/user_ptr.h +++ /dev/null @@ -1,35 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -#ifndef __ASM_USER_PTR_H -#define __ASM_USER_PTR_H
-#include <asm/morello.h>
-#ifdef CONFIG_CHERI_PURECAP_UABI
-static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) -{
- /*
* TODO [PCuABI] - the user root capability should be used, not the* kernel one.*/- uintcap_t root_cap = morello_get_root_cap();
Even this function morello_get_root_cap() can be removed now.
- return (void __user *)__builtin_cheri_address_set(root_cap, addr);
-} -#define uaddr_to_user_ptr(addr) uaddr_to_user_ptr(addr)
-static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) -{
- /*
* TODO [PCuABI] - the user root capability should be used, not the* kernel one.*/- uintcap_t root_cap = morello_get_root_cap();
- return (void __user *)__builtin_cheri_address_set(root_cap, addr);
-} -#define uaddr_to_user_ptr_safe(addr) uaddr_to_user_ptr_safe(addr)
-#endif /* CONFIG_CHERI_PURECAP_UABI */
-#endif /* __ASM_USER_PTR_H */
On 07/10/2022 15:27, Amit Kachhap wrote:
On 9/28/22 21:01, Kevin Brodsky wrote:
linux/user_ptr.h now uses generic PCuABI implementations of uaddr_to_user_ptr* and no longer includises asm/user_ptr.h. Time to remove it along with the corresponding config option.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/Kconfig | 3 --- arch/arm64/Kconfig | 1 - arch/arm64/include/asm/user_ptr.h | 35 ------------------------------- 3 files changed, 39 deletions(-) delete mode 100644 arch/arm64/include/asm/user_ptr.h
diff --git a/arch/Kconfig b/arch/Kconfig index b173a07c5a91..db0f652580c6 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1379,9 +1379,6 @@ config HAVE_ARCH_NODE_DEV_GROUP config ARCH_HAS_CHERI_H bool -config ARCH_HAS_USER_PTR_H - bool
config ARCH_HAS_CHERI_CAPABILITIES bool diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a0635a459a67..5bc5ab5e8689 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -30,7 +30,6 @@ config ARM64 select ARCH_HAS_GIGANTIC_PAGE select ARCH_HAS_KCOV select ARCH_HAS_CHERI_H - select ARCH_HAS_USER_PTR_H select ARCH_HAS_KEEPINITRD select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE diff --git a/arch/arm64/include/asm/user_ptr.h b/arch/arm64/include/asm/user_ptr.h deleted file mode 100644 index 323ad0301cbd..000000000000 --- a/arch/arm64/include/asm/user_ptr.h +++ /dev/null @@ -1,35 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -#ifndef __ASM_USER_PTR_H -#define __ASM_USER_PTR_H
-#include <asm/morello.h>
-#ifdef CONFIG_CHERI_PURECAP_UABI
-static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) -{ - /* - * TODO [PCuABI] - the user root capability should be used, not the - * kernel one. - */ - uintcap_t root_cap = morello_get_root_cap();
Even this function morello_get_root_cap() can be removed now.
Indeed that is planned :) More generally almost all uses of morello_root_cap should be removed, I'm planning this for either v2 or another series, see also the cover letter.
Kevin
- return (void __user *)__builtin_cheri_address_set(root_cap, addr); -} -#define uaddr_to_user_ptr(addr) uaddr_to_user_ptr(addr)
-static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) -{ - /* - * TODO [PCuABI] - the user root capability should be used, not the - * kernel one. - */ - uintcap_t root_cap = morello_get_root_cap();
- return (void __user *)__builtin_cheri_address_set(root_cap, addr); -} -#define uaddr_to_user_ptr_safe(addr) uaddr_to_user_ptr_safe(addr)
-#endif /* CONFIG_CHERI_PURECAP_UABI */
-#endif /* __ASM_USER_PTR_H */
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 + /** * 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 + * precise information for the pure-capability uABI. */ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr);
+/** + * 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); + +/** + * 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; + + return cheri_check_cap_data_access(ptr, len, cap_perms); +}
On 9/28/22 21:01, 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 suppose these names are Cheri neutral as far as possible. Also by looking at the Cheri Bsd, it implements coarse/block type permissions like CHERI_PERMS_KERNEL_DATA/CHERI_PERMS_KERNEL_CODE etc. Along with above we may also need coarse type permissions for binfmtelf.c.
- /**
- 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);
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
+/**
- 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);
How about check_privileged_user_ptr() ? This will make it consistent like above.
Thanks, Amit
- #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;- return cheri_check_cap_data_access(ptr, len, cap_perms);
+}
On 10/10/2022 11:43, Amit Kachhap wrote:
On 9/28/22 21:01, 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 suppose these names are Cheri neutral as far as possible.
That's the idea yes, unrelated to CHERI in fact.
Also by looking at the Cheri Bsd, it implements coarse/block type permissions like CHERI_PERMS_KERNEL_DATA/CHERI_PERMS_KERNEL_CODE etc.
These are capability permissions, so slightly different focus. What I have introduced in cheri.h (CHERI_PERMS_*) should cover most use-cases. I'm not convinced we need the same set of macros as in CheriBSD (i.e. you can obtain what you need by combining CHERI_PERMS_* and Global), but we shall see.
Along with above we may also need coarse type permissions for binfmtelf.c.
Are you thinking about something in particular, apart from AT_CHERI_* that will clearly make use of CHERI_PERMS_*?
/** * 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
- precise information for the pure-capability uABI.
*/ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr); +/**
- 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);
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
+/**
- 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);
How about check_privileged_user_ptr() ? This will make it consistent like above.
This is actually the opposite situation here: the user pointer comes from userspace and we want to check it, there is nothing privileged about the pointer (it is completely untrusted).
Kevin
Thanks, Amit
#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;
+ return cheri_check_cap_data_access(ptr, len, cap_perms); +}
On Thu, Oct 13, 2022 at 02:50:09PM +0200, Kevin Brodsky wrote:
On 10/10/2022 11:43, Amit Kachhap wrote:
On 9/28/22 21:01, 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 suppose these names are Cheri neutral as far as possible.
That's the idea yes, unrelated to CHERI in fact.
Also by looking at the Cheri Bsd, it implements coarse/block type permissions like CHERI_PERMS_KERNEL_DATA/CHERI_PERMS_KERNEL_CODE etc.
These are capability permissions, so slightly different focus. What I have introduced in cheri.h (CHERI_PERMS_*) should cover most use-cases. I'm not convinced we need the same set of macros as in CheriBSD (i.e. you can obtain what you need by combining CHERI_PERMS_* and Global), but we shall see.
Along with above we may also need coarse type permissions for binfmtelf.c.
Are you thinking about something in particular, apart from AT_CHERI_* that will clearly make use of CHERI_PERMS_*?
/** * 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
- precise information for the pure-capability uABI.
*/ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr); +/**
- 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);
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
--- BR B.
+/**
- 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);
How about check_privileged_user_ptr() ? This will make it consistent like above.
This is actually the opposite situation here: the user pointer comes from userspace and we want to check it, there is nothing privileged about the pointer (it is completely untrusted).
Kevin
Thanks, Amit
#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;
+ return cheri_check_cap_data_access(ptr, len, cap_perms); +}
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 18/10/2022 22:16, Beata Michalska wrote:
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
Kevin
On Fri, Oct 21, 2022 at 02:34:48PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:16, Beata Michalska wrote:
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
That still leaves it bit vague (as per your comment re: restricted bounds and permissions). This is tricky to get it right. 'managed' pop into my mind at some point: as the capability created will be handled internally by the kernel only, but it does not sound entirely appealing either. That said, I think I am out of 'reasonable' ideas at this point, sadly.
--- BR B.
Kevin
On 25/10/2022 16:32, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:34:48PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:16, Beata Michalska wrote:
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
That still leaves it bit vague (as per your comment re: restricted bounds and permissions). This is tricky to get it right. 'managed' pop into my mind at some point: as the capability created will be handled internally by the kernel only, but it does not sound entirely appealing either. That said, I think I am out of 'reasonable' ideas at this point, sadly.
I feel that "managed" is not much less vague than "loose", which I don't really like either right now...
In the end we should probably either have no annotation or one pointing to what we actually mean, so it could be something like "inkernel", "make_inkernel_user_ptr()"? It looks rather weird, but maybe that's the point, it might get the user or reader to check the function documentation :)
Kevin
On 01/11/2022 10:03, Kevin Brodsky wrote:
On 25/10/2022 16:32, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:34:48PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:16, Beata Michalska wrote:
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
That still leaves it bit vague (as per your comment re: restricted bounds and permissions). This is tricky to get it right. 'managed' pop into my mind at some point: as the capability created will be handled internally by the kernel only, but it does not sound entirely appealing either. That said, I think I am out of 'reasonable' ideas at this point, sadly.
I feel that "managed" is not much less vague than "loose", which I don't really like either right now...
In the end we should probably either have no annotation or one pointing to what we actually mean, so it could be something like "inkernel", "make_inkernel_user_ptr()"? It looks rather weird, but maybe that's the point, it might get the user or reader to check the function documentation :)
As you said above it is a subtle thing you want the name to convey, and I feel that while weird "make_inkernel_user_ptr()" might fit. At least it makes sense with the details above, and the name hints that it should be "in the kernel".
A bit of an in-between proposition compared to the previous ones : maybe "unsafe" could work ? From your explanation it is indeed unsafe to share to user space, even though it _is_ restricted. It can be loose, but loose itself is unsafe as well. It also hints that you should be wary while using this, but maybe too strongly once again ?
Lots of options have already been discussed so it's hard to think of others, but "inkernel" does feel like it's the closest approximation of what you want to convey. My "unsafe" idea is probably discouraging using the function too much.
Kevin
Regards, Téo
On 01/11/2022 15:11, Teo Couprie Diaz wrote:
On 01/11/2022 10:03, Kevin Brodsky wrote:
On 25/10/2022 16:32, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:34:48PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:16, Beata Michalska wrote:
> Minor: How about make_user_ptr_with_capability()? privileged may > confuse > with privilege mode or something else. Although I don't have strong > opinion here. I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
That still leaves it bit vague (as per your comment re: restricted bounds and permissions). This is tricky to get it right. 'managed' pop into my mind at some point: as the capability created will be handled internally by the kernel only, but it does not sound entirely appealing either. That said, I think I am out of 'reasonable' ideas at this point, sadly.
I feel that "managed" is not much less vague than "loose", which I don't really like either right now...
In the end we should probably either have no annotation or one pointing to what we actually mean, so it could be something like "inkernel", "make_inkernel_user_ptr()"? It looks rather weird, but maybe that's the point, it might get the user or reader to check the function documentation :)
As you said above it is a subtle thing you want the name to convey, and I feel that while weird "make_inkernel_user_ptr()" might fit. At least it makes sense with the details above, and the name hints that it should be "in the kernel".
A bit of an in-between proposition compared to the previous ones : maybe "unsafe" could work ? From your explanation it is indeed unsafe to share to user space, even though it _is_ restricted. It can be loose, but loose itself is unsafe as well. It also hints that you should be wary while using this, but maybe too strongly once again ?
Lots of options have already been discussed so it's hard to think of others, but "inkernel" does feel like it's the closest approximation of what you want to convey. My "unsafe" idea is probably discouraging using the function too much.
How about make_kernel_only_user_ptr()? The kernel only part seems to be the crux, and using the function would serve as a reminder of that.
Long shot, but I guess there is not many spare or unused bits around - otherwise we could have a kernel only flag + uaccess copy_to_user routines would fail if it was set to avoid leaking privileged caps.
Kevin
Regards, Téo _______________________________________________ linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 01/11/2022 19:16, Zachary Leaf wrote:
On 01/11/2022 15:11, Teo Couprie Diaz wrote:
On 01/11/2022 10:03, Kevin Brodsky wrote:
On 25/10/2022 16:32, Beata Michalska wrote:
On Fri, Oct 21, 2022 at 02:34:48PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:16, Beata Michalska wrote:
>> Minor: How about make_user_ptr_with_capability()? privileged may >> confuse >> with privilege mode or something else. Although I don't have strong >> opinion here. > I agree, "privileged" is a very overloaded term. What I am trying > to convey > is that the returned user pointer is to be used by the kernel > itself only, > because we do not control how the bounds are extended for > representability > purposes. This is fine inside the kernel as we generally cannot do > better > (and have enough privilege to access any part of the address > space), but it > is not if userspace gets hold of this capability, as it could give > access to > arbitrary objects in addition to the one we intended. > Additionally, we > provide all possible Load/Store capability permissions as > requested, which > is again fine when the kernel controls the access but not > necessarily if > userspace does (e.g. we may want to provide Load but not LoadCap). > > It is in this sense that the user pointer is "privileged" (because > only the > kernel should use it), but I agree that word is misleading. Maybe > this is > too subtle to convey in the naming. Since there are very few > places in which > the kernel creates user pointers for userspace (I think just > binfmt_elf + > mmap & co, where the CHERI API should be used directly instead of > this one), > the risks of accidental use of this function may be low enough > that we can > keep the name simple. Definitely looking for opinions on this! > Jumping here on this one to hopefully keep the discussion in a single thread...
So what we are looking at is capability with potentially elevated permissions and extended boundaries under sole kernel control... How about create_unrestricted_user_ptr instead ? or create_relaxed_user_ptr ?
"unrestricted" sounds deceiving as bounds and permissions are restricted, though not precisely. "relaxed" sounds a little bit strange for a pointer :D
That said these are inspiring, how about "loose"? "make_loose_user_ptr()"? It is hard to understand what that would refer to outside of the CHERI context, but I'm not sure we can do much better.
That still leaves it bit vague (as per your comment re: restricted bounds and permissions). This is tricky to get it right. 'managed' pop into my mind at some point: as the capability created will be handled internally by the kernel only, but it does not sound entirely appealing either. That said, I think I am out of 'reasonable' ideas at this point, sadly.
I feel that "managed" is not much less vague than "loose", which I don't really like either right now...
In the end we should probably either have no annotation or one pointing to what we actually mean, so it could be something like "inkernel", "make_inkernel_user_ptr()"? It looks rather weird, but maybe that's the point, it might get the user or reader to check the function documentation :)
As you said above it is a subtle thing you want the name to convey, and I feel that while weird "make_inkernel_user_ptr()" might fit. At least it makes sense with the details above, and the name hints that it should be "in the kernel".
A bit of an in-between proposition compared to the previous ones : maybe "unsafe" could work ? From your explanation it is indeed unsafe to share to user space, even though it _is_ restricted. It can be loose, but loose itself is unsafe as well. It also hints that you should be wary while using this, but maybe too strongly once again ?
Lots of options have already been discussed so it's hard to think of others, but "inkernel" does feel like it's the closest approximation of what you want to convey. My "unsafe" idea is probably discouraging using the function too much.
How about make_kernel_only_user_ptr()? The kernel only part seems to be the crux, and using the function would serve as a reminder of that.
Discussed this offline. Qualifying this function is particularly tricky as the meaning is so subtle (and new in a kernel context). In the end Téo's suggestion to use "unsafe" seems to be the most palatable as it does give a warning without being specific about what this warning is, inviting the user to read the function description. Therefore I'd go for make_user_ptr_unsafe().
Thanks everyone for the suggestions, that was very helpful!
Long shot, but I guess there is not many spare or unused bits around - otherwise we could have a kernel only flag + uaccess copy_to_user routines would fail if it was set to avoid leaking privileged caps.
This is quite an interesting idea, I hadn't considered that :) We only use one User bit at the moment so reserving another for internal kernel usage is plausible. The main trouble here is that explicit checking would be needed, adding a lot of branches to copy_to_user_with_ptr(). It might be acceptable if its performance is not too critical, but this is hard to say at this point. Worth keeping that idea in mind though!
Kevin
On 10/13/22 18:20, Kevin Brodsky wrote:
On 10/10/2022 11:43, Amit Kachhap wrote:
On 9/28/22 21:01, 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 suppose these names are Cheri neutral as far as possible.
That's the idea yes, unrelated to CHERI in fact.
Also by looking at the Cheri Bsd, it implements coarse/block type permissions like CHERI_PERMS_KERNEL_DATA/CHERI_PERMS_KERNEL_CODE etc.
These are capability permissions, so slightly different focus. What I have introduced in cheri.h (CHERI_PERMS_*) should cover most use-cases. I'm not convinced we need the same set of macros as in CheriBSD (i.e. you can obtain what you need by combining CHERI_PERMS_* and Global), but we shall see.
Along with above we may also need coarse type permissions for binfmtelf.c.
Are you thinking about something in particular, apart from AT_CHERI_* that will clearly make use of CHERI_PERMS_*?
Probably AT_CHERI_* definitions should suffice for us. Also I suppose it can be placed in generic include/linux/elf.h or something similar.
Amit
/** * 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
- precise information for the pure-capability uABI.
*/ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr); +/**
- 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);
Minor: How about make_user_ptr_with_capability()? privileged may confuse with privilege mode or something else. Although I don't have strong opinion here.
I agree, "privileged" is a very overloaded term. What I am trying to convey is that the returned user pointer is to be used by the kernel itself only, because we do not control how the bounds are extended for representability purposes. This is fine inside the kernel as we generally cannot do better (and have enough privilege to access any part of the address space), but it is not if userspace gets hold of this capability, as it could give access to arbitrary objects in addition to the one we intended. Additionally, we provide all possible Load/Store capability permissions as requested, which is again fine when the kernel controls the access but not necessarily if userspace does (e.g. we may want to provide Load but not LoadCap).
It is in this sense that the user pointer is "privileged" (because only the kernel should use it), but I agree that word is misleading. Maybe this is too subtle to convey in the naming. Since there are very few places in which the kernel creates user pointers for userspace (I think just binfmt_elf + mmap & co, where the CHERI API should be used directly instead of this one), the risks of accidental use of this function may be low enough that we can keep the name simple. Definitely looking for opinions on this!
+/**
- 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);
How about check_privileged_user_ptr() ? This will make it consistent like above.
This is actually the opposite situation here: the user pointer comes from userspace and we want to check it, there is nothing privileged about the pointer (it is completely untrusted).
Kevin
Thanks, Amit
#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;
+ return cheri_check_cap_data_access(ptr, len, cap_perms); +}
On 03/11/2022 11:20, Amit Kachhap wrote:
Along with above we may also need coarse type permissions for binfmtelf.c.
Are you thinking about something in particular, apart from AT_CHERI_* that will clearly make use of CHERI_PERMS_*?
Probably AT_CHERI_* definitions should suffice for us. Also I suppose it can be placed in generic include/linux/elf.h or something similar.
What do you think should be added to linux/elf.h? CHERI_PERMS_* is provided as part of linux/cheri.h along with the rest of the CHERI-specific definitions.
Kevin
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 ? 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. And what about execute permission?
/**
- 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
*/
- precise information for the pure-capability uABI.
void __user *uaddr_to_user_ptr_safe(ptraddr_t addr); +/**
- 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.
+/**
- 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.
- 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
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.
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.
/**
- 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.
+/**
- 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.
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
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
On 18/10/2022 22:15, Beata Michalska wrote:
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 quite like this actually, this avoids introducing these new flags that are unlikely to be used by any other function. Will ponder some more but I would probably go for that, thanks for the suggestion!
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)
Having wider capability permissions than MMU permissions is indeed unusual but is made possible by the new PROT_MAX() mechanism. An important difference is that capability permissions can never be increased, while MMU permissions are flexible. The typical example is a JIT writing code: the mapping is initially RW, then changed to RX, but X cannot be added to capability permissions so the capability needs to be RWX initially.
Something I wanted to insist on here is that these functions do not interact with mappings or PTEs at all, so for instance there is no concern with races. They are effectively pure functions (if you consider that the root capability is a constant).
Overall, my rambling here might be just that - pure rambling.
Definitely useful rambling :)
Kevin
On Fri, Oct 21, 2022 at 02:35:06PM +0200, Kevin Brodsky wrote:
On 18/10/2022 22:15, Beata Michalska wrote:
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 quite like this actually, this avoids introducing these new flags that are unlikely to be used by any other function. Will ponder some more but I would probably go for that, thanks for the suggestion!
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)
Having wider capability permissions than MMU permissions is indeed unusual but is made possible by the new PROT_MAX() mechanism. An important difference is that capability permissions can never be increased, while MMU permissions are flexible. The typical example is a JIT writing code: the mapping is initially RW, then changed to RX, but X cannot be added to capability permissions so the capability needs to be RWX initially.
Something I wanted to insist on here is that these functions do not interact with mappings or PTEs at all, so for instance there is no concern with races. They are effectively pure functions (if you consider that the root capability is a constant).
Thanks for the clarification. I might be finally catching up with the concepts behind the changes proposed.
--- BR B.
Overall, my rambling here might be just that - pure rambling.
Definitely useful rambling :)
Kevin
cow_user_page() needs to create a user pointer from scratch in order to copy data from an existing user page. To make this safer in PCuABI, create a user (capability) pointer with appropriate bounds and permissions, preventing invalid accesses to user memory.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
Just an example of what replacing uaddr_to_user_ptr_safe() with make_privileged_user_ptr() looks like, not to be merged on its own.
mm/memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/memory.c b/mm/memory.c index ab7e87de3314..7ed4365b628d 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2778,7 +2778,8 @@ static inline bool cow_user_page(struct page *dst, struct page *src, * fails, we just zero-fill it. Live with it. */ kaddr = kmap_atomic(dst); - uaddr = uaddr_to_user_ptr_safe(addr & PAGE_MASK); + uaddr = make_privileged_user_ptr(addr & PAGE_MASK, PAGE_SIZE, + USER_PTR_CAN_READ);
/* * On architectures with software "accessed" bits, we would
Hi Kevin,
On 28-09-2022 16:31, Kevin Brodsky wrote:
Hi,
I am posting this series now to gather some opinions (notably in terms of naming) before I proceed further.
The main focus is the introduction of two new user_ptr helpers: make_privileged_user_ptr() to create fine-grained user pointers (appropriate bounds and permissions), and check_user_ptr() to check user pointers. This does however require more involved CHERI operations than what we've used so far, and as a result it felt like the right time to introduce a new header with various CHERI-related definitions.
This new cheri.h header should be included in new code instead of the compiler-provided cheriintrin.h, notably because it is safe to include it unconditionally. linux/cheri.h is also a great place to introduce appropriate (CHERI-generic) root capabilities, which is another focus of this series. This makes it possible to have generic implementations of uaddr_to_user_ptr*() and get rid of asm/user_ptr.h.
The introduction of a root userspace capability with appropriate bounds and permissions is the only functional change from a userspace perspective: many capabilities given to userspace will now have bounds encompassing only the user address space and permissions corresponding to what is expected of an RWX capability in PCuABI. This work is to be continued by replacing most uses of morello_root_cap with cheri_root_cap_userspace (either in v2 or in a separate series).
On a similar theme, compat_ptr() should be modified to derive capabilities from the current user DDC, and the new seal/CID root capabilities should be used in binfmt_elf.c. This would complete the transition to appropriate root capabilities.
Back to the two new user_ptr helpers, make_privileged_user_ptr() is meant to replace uaddr_to_user_ptr_safe() and the latter should eventually disappear. This probably belongs to a different patch series, however the last patch provides an example of such a change. This work should probably wait until we start accessing user memory through capabilities in uaccess, as right now the capability metadata is not used anyway. Note that calls to uaddr_to_user_ptr() are workarounds in themselves and should all be eliminated eventually, so they are not considered here. Regarding check_user_ptr(), there is no immediate need for it - it will become relevant to implement explicit checking of user pointers (when get_user_pages() and friends are used).
Finally the user_ptr.rst documentation needs to be updated to reflect the new helpers, this is to be done in v2.
This series depends on Beata's handy printk patch for the warning messages. It was lightly tested and should be mostly fine, however note that compat_ptr() currently triggers warnings because it is implemented in terms of uaddr_to_user_ptr_safe() and compat_ptr() may be passed arbitrary integers. This will be fixed in v2 by appropriately deriving capabilities from DDC as mentioned above.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/cheri_ptr_api
Thanks, Kevin
The series looks great! The naming makes sense to me and the structure of the new additions looks good.
Thanks, Tudor
Kevin Brodsky (9): linux/user_ptr.h: Remove kaddr_to_user_ptr() linux/user_ptr.h: Improve comment formatting arm64: uapi: Add asm/cheri.h linux/cheri.h: Introduce CHERI helpers arm64: morello: Implement cheri.h linux/user_ptr.h: Generic PCuABI impl for uaddr_to_user_ptr* arm64: Remove asm/user_ptr.h linux/user_ptr.h: Introduce fine-grained helpers mm/memory: Create fine-grained user pointer
Documentation/core-api/user_ptr.rst | 8 -- arch/Kconfig | 2 +- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/cheri.h | 14 ++++ arch/arm64/include/asm/user_ptr.h | 43 ---------- arch/arm64/include/uapi/asm/cheri.h | 7 ++ arch/arm64/kernel/morello.c | 39 +++++++-- include/linux/cheri.h | 122 ++++++++++++++++++++++++++++ include/linux/user_ptr.h | 113 +++++++++++++++++++------- lib/Makefile | 3 + lib/cheri.c | 67 +++++++++++++++ lib/user_ptr.c | 62 ++++++++++++++ mm/memory.c | 3 +- 13 files changed, 392 insertions(+), 93 deletions(-) create mode 100644 arch/arm64/include/asm/cheri.h delete mode 100644 arch/arm64/include/asm/user_ptr.h create mode 100644 arch/arm64/include/uapi/asm/cheri.h create mode 100644 include/linux/cheri.h create mode 100644 lib/cheri.c create mode 100644 lib/user_ptr.c
Hi Kevin,
I must admit I did a rather quick scan of the changes with few comments around naming and some functionality introduced. No major issues, just few bits that I am most probably missing the context on. Anyways, thanks for pulling it up - it's good to see the direction of the changes to come inevitably.
--- BR B.
On Wed, Sep 28, 2022 at 04:31:39PM +0100, Kevin Brodsky wrote:
Hi,
I am posting this series now to gather some opinions (notably in terms of naming) before I proceed further.
The main focus is the introduction of two new user_ptr helpers: make_privileged_user_ptr() to create fine-grained user pointers (appropriate bounds and permissions), and check_user_ptr() to check user pointers. This does however require more involved CHERI operations than what we've used so far, and as a result it felt like the right time to introduce a new header with various CHERI-related definitions.
This new cheri.h header should be included in new code instead of the compiler-provided cheriintrin.h, notably because it is safe to include it unconditionally. linux/cheri.h is also a great place to introduce appropriate (CHERI-generic) root capabilities, which is another focus of this series. This makes it possible to have generic implementations of uaddr_to_user_ptr*() and get rid of asm/user_ptr.h.
The introduction of a root userspace capability with appropriate bounds and permissions is the only functional change from a userspace perspective: many capabilities given to userspace will now have bounds encompassing only the user address space and permissions corresponding to what is expected of an RWX capability in PCuABI. This work is to be continued by replacing most uses of morello_root_cap with cheri_root_cap_userspace (either in v2 or in a separate series).
On a similar theme, compat_ptr() should be modified to derive capabilities from the current user DDC, and the new seal/CID root capabilities should be used in binfmt_elf.c. This would complete the transition to appropriate root capabilities.
Back to the two new user_ptr helpers, make_privileged_user_ptr() is meant to replace uaddr_to_user_ptr_safe() and the latter should eventually disappear. This probably belongs to a different patch series, however the last patch provides an example of such a change. This work should probably wait until we start accessing user memory through capabilities in uaccess, as right now the capability metadata is not used anyway. Note that calls to uaddr_to_user_ptr() are workarounds in themselves and should all be eliminated eventually, so they are not considered here. Regarding check_user_ptr(), there is no immediate need for it - it will become relevant to implement explicit checking of user pointers (when get_user_pages() and friends are used).
Finally the user_ptr.rst documentation needs to be updated to reflect the new helpers, this is to be done in v2.
This series depends on Beata's handy printk patch for the warning messages. It was lightly tested and should be mostly fine, however note that compat_ptr() currently triggers warnings because it is implemented in terms of uaddr_to_user_ptr_safe() and compat_ptr() may be passed arbitrary integers. This will be fixed in v2 by appropriately deriving capabilities from DDC as mentioned above.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/cheri_ptr_api
Thanks, Kevin
Kevin Brodsky (9): linux/user_ptr.h: Remove kaddr_to_user_ptr() linux/user_ptr.h: Improve comment formatting arm64: uapi: Add asm/cheri.h linux/cheri.h: Introduce CHERI helpers arm64: morello: Implement cheri.h linux/user_ptr.h: Generic PCuABI impl for uaddr_to_user_ptr* arm64: Remove asm/user_ptr.h linux/user_ptr.h: Introduce fine-grained helpers mm/memory: Create fine-grained user pointer
Documentation/core-api/user_ptr.rst | 8 -- arch/Kconfig | 2 +- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/cheri.h | 14 ++++ arch/arm64/include/asm/user_ptr.h | 43 ---------- arch/arm64/include/uapi/asm/cheri.h | 7 ++ arch/arm64/kernel/morello.c | 39 +++++++-- include/linux/cheri.h | 122 ++++++++++++++++++++++++++++ include/linux/user_ptr.h | 113 +++++++++++++++++++------- lib/Makefile | 3 + lib/cheri.c | 67 +++++++++++++++ lib/user_ptr.c | 62 ++++++++++++++ mm/memory.c | 3 +- 13 files changed, 392 insertions(+), 93 deletions(-) create mode 100644 arch/arm64/include/asm/cheri.h delete mode 100644 arch/arm64/include/asm/user_ptr.h create mode 100644 arch/arm64/include/uapi/asm/cheri.h create mode 100644 include/linux/cheri.h create mode 100644 lib/cheri.c create mode 100644 lib/user_ptr.c
-- 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@op-lists.linaro.org
-
Amit Kachhap -
Beata Michalska -
Kevin Brodsky -
Kristina Martsenko -
Teo Couprie Diaz -
Tudor Cretu -
Zachary Leaf