In PCuABI, we need compat_ptr() to create a valid capability from an arbitrary compat pointer, to enable uaccess to the underlying memory. However, if the compat pointer is clearly an invalid address, it is preferable to return a null-derived (invalid) capability. This is especially true when the address is null: we clearly want to return the null capability in that case.
This should be a non-functional change (uaccess would fail anyway due to the absence of underlying mapping), but it avoids some confusion when debugging compat64 handling (and reduces the creation of capabilities).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/null_co...
arch/arm64/include/asm/compat.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 4556bed1704f..acf5487985b5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -93,7 +93,9 @@ static inline void __user *compat_ptr(compat_uptr_t uptr) * TODO [Morello] - this should be done using the current user DDC, not * the root user capability. */ - return (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr); + return likely(uptr >= PAGE_SIZE && uptr < TASK_SIZE_MAX) ? + (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr) : + as_user_ptr(uptr); } #define compat_ptr(uptr) compat_ptr(uptr) #endif
On 3/15/24 1:50 PM, Kevin Brodsky wrote:
In PCuABI, we need compat_ptr() to create a valid capability from an arbitrary compat pointer, to enable uaccess to the underlying memory. However, if the compat pointer is clearly an invalid address, it is preferable to return a null-derived (invalid) capability. This is especially true when the address is null: we clearly want to return the null capability in that case.
This should be a non-functional change (uaccess would fail anyway due to the absence of underlying mapping), but it avoids some confusion when debugging compat64 handling (and reduces the creation of capabilities).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/null_co...
arch/arm64/include/asm/compat.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 4556bed1704f..acf5487985b5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -93,7 +93,9 @@ static inline void __user *compat_ptr(compat_uptr_t uptr) * TODO [Morello] - this should be done using the current user DDC, not * the root user capability. */
- return (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr);
- return likely(uptr >= PAGE_SIZE && uptr < TASK_SIZE_MAX) ?
(void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr) :as_user_ptr(uptr);
the only issue might be "it's not as fast" as it has to do more, but reality s being more right is more important than more fast here imho, so i see no issue with this - some obvious invalid ptr range checking seems fine.
+1
On 05/04/2024 15:42, Carsten Haitzler wrote:
On 3/15/24 1:50 PM, Kevin Brodsky wrote:
In PCuABI, we need compat_ptr() to create a valid capability from an arbitrary compat pointer, to enable uaccess to the underlying memory. However, if the compat pointer is clearly an invalid address, it is preferable to return a null-derived (invalid) capability. This is especially true when the address is null: we clearly want to return the null capability in that case.
This should be a non-functional change (uaccess would fail anyway due to the absence of underlying mapping), but it avoids some confusion when debugging compat64 handling (and reduces the creation of capabilities).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/null_co...
arch/arm64/include/asm/compat.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 4556bed1704f..acf5487985b5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -93,7 +93,9 @@ static inline void __user *compat_ptr(compat_uptr_t uptr) * TODO [Morello] - this should be done using the current user DDC, not * the root user capability. */ - return (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr); + return likely(uptr >= PAGE_SIZE && uptr < TASK_SIZE_MAX) ? + (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr) : + as_user_ptr(uptr); } #define compat_ptr(uptr) compat_ptr(uptr) #endif
the only issue might be "it's not as fast" as it has to do more, but reality s being more right is more important than more fast here imho, so i see no issue with this - some obvious invalid ptr range checking seems fine.
Yes, that is a fair point. In fact that's part of the reason why I kept the bounds constant (I considered using mmap_min_addr as lower bound at some point, though it wouldn't work for other reasons). As it stands the overhead is probably unnoticeable (just some arithmetic and a couple of branches that are easily predicted).
To do the really correct thing, we should use the current user DDC here (TODO above), but that may be a bigger overhead.
+1
Thanks for the review! Now in next.
Kevin
On 15/03/2024 14:50, Kevin Brodsky wrote:
In PCuABI, we need compat_ptr() to create a valid capability from an arbitrary compat pointer, to enable uaccess to the underlying memory. However, if the compat pointer is clearly an invalid address, it is preferable to return a null-derived (invalid) capability. This is especially true when the address is null: we clearly want to return the null capability in that case.
This should be a non-functional change (uaccess would fail anyway due to the absence of underlying mapping), but it avoids some confusion when debugging compat64 handling (and reduces the creation of capabilities).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/null_co...
arch/arm64/include/asm/compat.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 4556bed1704f..acf5487985b5 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -93,7 +93,9 @@ static inline void __user *compat_ptr(compat_uptr_t uptr) * TODO [Morello] - this should be done using the current user DDC, not * the root user capability. */
- return (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr);
- return likely(uptr >= PAGE_SIZE && uptr < TASK_SIZE_MAX) ?
(void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr) :as_user_ptr(uptr);} #define compat_ptr(uptr) compat_ptr(uptr) #endif
Just a note that I had to amend and reland this patch. I had (once again) forgotten that user addresses cannot be directly compared in this manner - untagged_addr() must be used first to mask out the top byte. Android likes to insert dummy tags in the top byte (when not using MTE), and this patch crashes Android very early without the appropriate masking. The new diff can be found below.
Kevin
-------8<-------
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 4556bed1704f..4f6d7eb0106e 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -89,11 +89,15 @@ struct compat_statfs { #ifdef CONFIG_CHERI_PURECAP_UABI static inline void __user *compat_ptr(compat_uptr_t uptr) { + ptraddr_t addr = untagged_addr(uptr); + /* * TODO [Morello] - this should be done using the current user DDC, not * the root user capability. */ - return (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr); + return likely(addr >= PAGE_SIZE && addr < TASK_SIZE_MAX) ? + (void __user *)cheri_address_set(cheri_user_root_allperms_cap, uptr) : + as_user_ptr(uptr); } #define compat_ptr(uptr) compat_ptr(uptr) #endif
linux-morello@op-lists.linaro.org
-
Carsten Haitzler -
Kevin Brodsky