Hi,
This is a small update to the series, for more info see the original cover letter [1].
Following various discussions, the interface remains broadly unchanged, with a separate function for each permission set (read/write/RW). Luca's work on explicit checking however exposed that check_user_ptr_read() should clearly take a pointer to const. The write/RW variants still take a non-const pointer; accordingly fault_in_safe_writeable() will be changed to take a non-const pointer too [2], which feels more logical.
For symmetry, make_user_ptr_for_read_uaccess() now returns a pointer to const. A few patches are modified accordingly; the additional changes are minimal. A nice side-effect is that this reduces the risks of misuse at compile time, since copy_to_user(uptr, ...) will trigger a warning if uptr is a pointer to const.
v1..v2: - Patch 1: made check_user_ptr_read() and make_user_ptr_for_read_uaccess() take/return a pointer to const. - Patch 3, 4, 9: made variables pointers to const when using make_user_ptr_for_read_uaccess().
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/user_pt...
Thanks, Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Kevin Brodsky (9): linux/user_ptr.h: Introduce uaccess-related helpers fs/binfmt_elf: Create appropriate user pointer for uaccess coredump: Create appropriate user pointer for uaccess mm/memory: Create appropriate user pointer for uaccess Revert "mm/hugetlb: Use appropriate user pointer conversions" Revert "mm/shmem: Use appropriate user pointer conversions" audit: Create appropriate user pointer for uaccess perf: Avoid uaddr_to_user_ptr_safe() for arbitrary user address arm64: Create appropriate user pointer for uaccess
Documentation/core-api/user_ptr.rst | 100 ++++++++++++++++++---------- arch/arm64/kernel/debug-monitors.c | 3 +- arch/arm64/kernel/traps.c | 3 +- fs/binfmt_elf.c | 14 ++-- fs/coredump.c | 4 +- include/linux/user_ptr.h | 86 ++++++++++++++++++++++-- kernel/auditsc.c | 3 +- kernel/events/internal.h | 2 +- lib/user_ptr.c | 46 +++++++++++++ mm/hugetlb.c | 2 +- mm/memory.c | 4 +- mm/shmem.c | 2 +- 12 files changed, 218 insertions(+), 51 deletions(-)
Introduce new helpers that help interact with user pointers for the purpose of accessing user memory, in a PCuABI-friendly way:
* make_user_ptr_for_<perms>_uaccess() creates a user capability with the requested bounds and permissions, strictly to perform uaccess.
* check_user_ptr_<perms>() checks that the capability is valid and its bounds and permissions are appropriate for the requested access. This is needed if (and only if) user memory is accessed via kernel mappings, and capability metadata therefore needs to be checked upfront in PCuABI. When normal uaccess is performed, capability metadata is checked directly by the hardware; these helpers can be seen as the counterparts of such checks for "indirect uaccess" via kernel mappings.
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 specified length and permissions are ignored).
make_user_ptr_for_<perms>_uaccess() replaces uaddr_to_user_ptr_safe() to create user pointers to perform uaccess. The latter is still used (for the time being) to provide new user pointers to userspace.
The comments and documentation are updated accordingly.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- Documentation/core-api/user_ptr.rst | 100 ++++++++++++++++++---------- include/linux/user_ptr.h | 86 ++++++++++++++++++++++-- lib/user_ptr.c | 46 +++++++++++++ 3 files changed, 194 insertions(+), 38 deletions(-)
diff --git a/Documentation/core-api/user_ptr.rst b/Documentation/core-api/user_ptr.rst index 849433096b65..78aef3fbb753 100644 --- a/Documentation/core-api/user_ptr.rst +++ b/Documentation/core-api/user_ptr.rst @@ -78,7 +78,8 @@ Each function covers a particular category of input integer: * **Address**
- User-provided user address: ``uaddr_to_user_ptr()`` - - Kernel-controlled user address: ``uaddr_to_user_ptr_safe()`` + - Kernel-controlled user address: ``make_user_ptr_for_*_uaccess()``, + ``uaddr_to_user_ptr_safe()`` (see notes in the table below)
* **Compat pointer**: ``compat_ptr()``
@@ -103,39 +104,57 @@ PCuABI. The table below provides additional information about these functions, as well as the base capability that the user pointer is derived from in the PCuABI case.
-+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| Name | Suitable input | Example of input | Capability derived from | Notes | -+==============================+====================+========================+===================================+======================================================+ -| ``uaddr_to_user_ptr()`` | User-provided | Address stored in | By default, user root capability. | Using this function weakens the enforcement of the | -| | address | __u64 field of a | This could be modified for | capability model, as it allows a process to trigger | -| | | user-provided struct | testing purposes (e.g. null | accesses to its own memory without an appropriate | -| | | | capability to prevent such | capability. | -| | | | capabilities from being created | It is therefore only a stopgap while waiting for a | -| | | | at runtime). | uapi change allowing userspace to provide an actual | -| | | | | pointer instead of an address. | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``uaddr_to_user_ptr_safe()`` | Kernel-controlled | Address of new user | User root capability | This function should only be used in cases where the | -| | user address | mappings during | | kernel needs to access user memory using a bare | -| | | process initialisation | | virtual address that is not provided by userspace. | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``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 | | | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``as_user_ptr()`` | Arbitrary integer | Error code | Null capability | This is a pure representation change, as suggested | -| | | | | by the ``as_`` prefix. Returns up to 64 bits of an | -| | | | | arbitrary integer represented as a user pointer. The | -| | | | | result is not a valid pointer and cannot be | -| | | | | dereferenced. | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``u64_to_user_ptr()`` | ``u64`` integer | [Deprecated] | Null capability | Legacy function, new callers should not be added. | -| | | | | Existing callers should move to either | -| | | | | ``as_user_ptr()`` if the user pointer is not used to | -| | | | | access memory, or ``uaddr_to_user_ptr()`` if the | -| | | | | input is an address and the user pointer is | -| | | | | dereferenced (or ideally removed if the uapi can be | -| | | | | changed appropriately). | -+------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| Name | Suitable input | Example of input | Capability derived from | Notes | ++===================================+====================+===========================+===================================+======================================================+ +| ``uaddr_to_user_ptr()`` | User-provided | Address stored in | By default, user root capability. | Using this function weakens the enforcement of the | +| | address | ``__u64`` field of a | This could be modified for | capability model, as it allows a process to trigger | +| | | user-provided struct | testing purposes (e.g. null | accesses to its own memory without an appropriate | +| | | | capability to prevent such | capability. | +| | | | capabilities from being created | | +| | | | at runtime). | It is therefore only a stopgap while waiting for a | +| | | | | uapi change allowing userspace to provide an actual | +| | | | | pointer instead of an address. | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| ``uaddr_to_user_ptr_safe()`` | Kernel-controlled | Address of a user mapping | User root capability | This function should only be used in the few cases | +| | user address | | | where the kernel needs to **provide** a new pointer | +| | | | | to userspace, starting from an address. A typical | +| | | | | example is returning a pointer to a newly created | +| | | | | mapping in ``mmap()``. It should not be used to | +| | | | | perform uaccess, ``make_user_ptr_for_*_uaccess()`` | +| | | | | should be used for that purpose. | +| | | | | | +| | | | | Like ``uaddr_to_user_ptr()``, this function is a | +| | | | | stopgap. In PCuABI, capability pointers provided to | +| | | | | userspace have well-defined bounds and permissions, | +| | | | | and these will eventually be set by dedicated code, | +| | | | | removing the need for this function. | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| ``make_user_ptr_for_*_uaccess()`` | Kernel-controlled | Address of a user mapping | User root capability | This function should be used when the kernel needs | +| | user address | | | to access user memory at a certain address. That | +| | | | | address must be determined by the kernel itself, | +| | | | | typically from mm information (start address of a | +| | | | | mapping, address of program arguments, etc.). | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| ``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 | | | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| ``as_user_ptr()`` | Arbitrary integer | Error code | Null capability | This is a pure representation change, as suggested | +| | | | | by the ``as_`` prefix. Returns up to 64 bits of an | +| | | | | arbitrary integer represented as a user pointer. The | +| | | | | result is not a valid pointer and cannot be | +| | | | | dereferenced. | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+ +| ``u64_to_user_ptr()`` | ``u64`` integer | [Deprecated] | Null capability | Legacy function, new callers should not be added. | +| | | | | | +| | | | | Existing callers should move to either | +| | | | | ``as_user_ptr()`` if the user pointer is not used to | +| | | | | access memory, or ``uaddr_to_user_ptr()`` if the | +| | | | | input is an address and the user pointer is | +| | | | | dereferenced (or ideally removed if the uapi can be | +| | | | | changed appropriately). | ++-----------------------------------+--------------------+---------------------------+-----------------------------------+------------------------------------------------------+
+-----------------------------------------------------------------------+ @@ -218,6 +237,19 @@ preserving their metadata in PCuABI).
* ``USER_PTR_PAGE_ALIGN(p)``
+Explicit checking +----------------- + +In a few situations, typically when user memory is accessed via a kernel +mapping, it may be necessary to explicitly check that a user pointer allows +a given type of access to a given range, without dereferencing it. + +This can be done using the ``check_user_ptr_*()`` functions, see +``<linux/user_ptr.h>`` for details on how to use them. + +Note that these functions are no-ops (always succeed) when PCuABI is not +selected, as there is no user pointer metadata to check in that case. + Other functions handling user pointers --------------------------------------
diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 21070de7c17c..2c2180f0f0c3 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -43,17 +43,69 @@ void __user *uaddr_to_user_ptr(ptraddr_t addr);
/** * uaddr_to_user_ptr_safe() - Convert a kernel-generated user address to a - * user pointer. + * user pointer. * @addr: The address to set the pointer to. * * Return: 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 (i.e. it is not provided by userspace). + * This function should be used when a new user pointer needs to be provided to + * userspace. @addr should be controlled by the kernel (i.e. not an arbitrary + * user-provided value). + * + * When a user pointer is needed to access user memory (in-kernel use), + * make_user_ptr_for_*_uaccess() should be used instead. + * + * All uses of this function should eventually be replaced by dedicated code + * ensuring that the bounds and permissions of the user capability are minimised + * in the pure-capability ABI. */ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr);
+/** + * make_user_ptr_for_<perms>_uaccess() - Create a user pointer from kernel-generated + * parameters, to access user memory. + * @addr: The address to set the pointer to. + * @len: The minimum size of the region the pointer should allow to access. + * + * Return: A user pointer with its address set to @addr. + * + * These functions should be used when a user pointer is required because user + * memory at a certain address needs to be accessed. The parameters should not + * originate from 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 appropriately for each function (read/write/RW). + */ +const void __user *make_user_ptr_for_read_uaccess(ptraddr_t addr, size_t len); +void __user *make_user_ptr_for_write_uaccess(ptraddr_t addr, size_t len); +void __user *make_user_ptr_for_rw_uaccess(ptraddr_t addr, size_t len); + +/** + * check_user_ptr_<perms>() - 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_*. + * + * Checks 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 calling the appropriate function (read/write/RW). + * + * These functions only check 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 + * true is always returned. + * + * Return: true if @ptr passes the checks. + */ +bool check_user_ptr_read(const void __user *ptr, size_t len); +bool check_user_ptr_write(void __user *ptr, size_t len); +bool check_user_ptr_rw(void __user *ptr, size_t len); + #else /* CONFIG_CHERI_PURECAP_UABI */
static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) @@ -66,6 +118,32 @@ static inline void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) return as_user_ptr(addr); }
+static inline const void __user *make_user_ptr_for_read_uaccess(ptraddr_t addr, size_t len) +{ + return as_user_ptr(addr); +} +static inline void __user *make_user_ptr_for_write_uaccess(ptraddr_t addr, size_t len) +{ + return as_user_ptr(addr); +} +static inline void __user *make_user_ptr_for_rw_uaccess(ptraddr_t addr, size_t len) +{ + return as_user_ptr(addr); +} + +static inline bool check_user_ptr_read(const void __user *ptr, size_t len) +{ + return true; +} +static inline bool check_user_ptr_write(void __user *ptr, size_t len) +{ + return true; +} +static inline bool check_user_ptr_rw(void __user *ptr, size_t len) +{ + return true; +} + #endif /* CONFIG_CHERI_PURECAP_UABI */
/** diff --git a/lib/user_ptr.c b/lib/user_ptr.c index 7fd67f793122..115efc9fe678 100644 --- a/lib/user_ptr.c +++ b/lib/user_ptr.c @@ -24,3 +24,49 @@ void __user *uaddr_to_user_ptr_safe(ptraddr_t addr) return ret; }
+/* + * Grant all permissions in each category, e.g. loading/storing capabilities in + * addition to standard data. + */ +const void __user *make_user_ptr_for_read_uaccess(ptraddr_t addr, size_t len) +{ + cheri_perms_t cap_perms = CHERI_PERM_GLOBAL | CHERI_PERMS_READ; + + return cheri_build_user_cap_inexact_bounds(addr, len, cap_perms); +} + +void __user *make_user_ptr_for_write_uaccess(ptraddr_t addr, size_t len) +{ + cheri_perms_t cap_perms = CHERI_PERM_GLOBAL | CHERI_PERMS_WRITE; + + return cheri_build_user_cap_inexact_bounds(addr, len, cap_perms); +} + +void __user *make_user_ptr_for_rw_uaccess(ptraddr_t addr, size_t len) +{ + cheri_perms_t cap_perms = CHERI_PERM_GLOBAL | CHERI_PERMS_READ + | CHERI_PERMS_WRITE; + + return cheri_build_user_cap_inexact_bounds(addr, len, cap_perms); +} + +/* + * Only check whether the capability has the minimal data permissions (Load / + * Store). The underlying assumption is that these functions are 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. + */ +bool check_user_ptr_read(const void __user *ptr, size_t len) +{ + return cheri_check_cap(ptr, len, CHERI_PERM_LOAD); +} + +bool check_user_ptr_write(void __user *ptr, size_t len) +{ + return cheri_check_cap(ptr, len, CHERI_PERM_STORE); +} + +bool check_user_ptr_rw(void __user *ptr, size_t len) +{ + return cheri_check_cap(ptr, len, CHERI_PERM_LOAD | CHERI_PERM_STORE); +}
The ELF loader needs to zero out part of the BSS explicitly. It also needs to read the program arguments directly from user memory when dumping a core file. In both cases we only have the address of the memory mapping / arguments to access, and need to create a valid user pointer to perform the uaccess.
uaddr_to_user_ptr_safe() should no longer be used for that purpose. Instead, we use make_user_ptr_for_{read,write}_uaccess() to create a user pointer with appropriate bounds and permissions (in PCuABI).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- fs/binfmt_elf.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 1d82465cb9e9..564aebce488c 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -144,7 +144,8 @@ static int padzero(unsigned long elf_bss) nbyte = ELF_PAGEOFFSET(elf_bss); if (nbyte) { nbyte = ELF_MIN_ALIGN - nbyte; - if (clear_user(uaddr_to_user_ptr_safe(elf_bss), nbyte)) + if (clear_user(make_user_ptr_for_write_uaccess(elf_bss, nbyte), + nbyte)) return -EFAULT; } return 0; @@ -1132,11 +1133,15 @@ static int load_elf_binary(struct linux_binprm *bprm) goto out_free_dentry; nbyte = ELF_PAGEOFFSET(elf_bss); if (nbyte) { + void __user *uptr; + nbyte = ELF_MIN_ALIGN - nbyte; if (nbyte > elf_brk - elf_bss) nbyte = elf_brk - elf_bss; - if (clear_user(uaddr_to_user_ptr_safe(elf_bss + - load_bias), nbyte)) { + + uptr = make_user_ptr_for_write_uaccess( + elf_bss + load_bias, nbyte); + if (clear_user(uptr, nbyte)) { /* * This bss-zeroing can fail if the ELF * file specifies odd protections. So @@ -1674,7 +1679,8 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p, if (len >= ELF_PRARGSZ) len = ELF_PRARGSZ-1; if (copy_from_user(&psinfo->pr_psargs, - uaddr_to_user_ptr_safe(mm->arg_start), len)) + make_user_ptr_for_read_uaccess(mm->arg_start, len), + len)) return -EFAULT; for(i = 0; i < len; i++) if (psinfo->pr_psargs[i] == 0)
dump_vma_snapshot() wants to read some memory in a user mapping; to perform the uaccess we therefore need to create a valid user pointer from the mapping address.
uaddr_to_user_ptr_safe() should no longer be used for that purpose. Instead, we use make_user_ptr_for_read_uaccess() to create a user pointer with appropriate bounds and permissions (in PCuABI).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- fs/coredump.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/coredump.c b/fs/coredump.c index 90009027a8af..e02501be38f1 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -1181,8 +1181,10 @@ static bool dump_vma_snapshot(struct coredump_params *cprm)
if (m->dump_size == DUMP_SIZE_MAYBE_ELFHDR_PLACEHOLDER) { char elfmag[SELFMAG]; + const void __user *uptr = make_user_ptr_for_read_uaccess( + m->start, SELFMAG);
- if (copy_from_user(elfmag, uaddr_to_user_ptr_safe(m->start), SELFMAG) || + if (copy_from_user(elfmag, uptr, SELFMAG) || memcmp(elfmag, ELFMAG, SELFMAG) != 0) { m->dump_size = 0; } else {
__wp_page_copy_user() sometimes needs to read a page directly via a user mapping; to perform the uaccess we therefore need to create a valid user pointer from the address of the page.
uaddr_to_user_ptr_safe() should no longer be used for that purpose. Instead, we use make_user_ptr_for_read_uaccess() to create a user pointer with appropriate bounds and permissions (in PCuABI).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- mm/memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c index 3e4490f4ab6e..e3cc37514a33 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2852,7 +2852,7 @@ static inline bool __wp_page_copy_user(struct page *dst, struct page *src, { bool ret; void *kaddr; - void __user *uaddr; + const void __user *uaddr; bool locked = false; struct vm_area_struct *vma = vmf->vma; struct mm_struct *mm = vma->vm_mm; @@ -2870,7 +2870,7 @@ static inline bool __wp_page_copy_user(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_user_ptr_for_read_uaccess(addr & PAGE_MASK, PAGE_SIZE);
/* * On architectures with software "accessed" bits, we would
We do not currently support usefaultfd in PCuABI, and uaddr_to_user_ptr_safe() should not be used to create a user pointer from an arbitrary (user-provided) address. It is unclear whether userfaultfd can be supported in a safe way in PCuABI, for now let's return this code to its original state.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- mm/hugetlb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dffda44762d5..e36ca75311a5 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6041,7 +6041,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, }
ret = copy_huge_page_from_user(page, - uaddr_to_user_ptr_safe(src_addr), + (const void __user *) src_addr, pages_per_huge_page(h), false);
/* fallback to copy_from_user outside mmap_lock */
We do not currently support usefaultfd in PCuABI, and uaddr_to_user_ptr_safe() should not be used to create a user pointer from an arbitrary (user-provided) address. It is unclear whether userfaultfd can be supported in a safe way in PCuABI, for now let's return this code to its original state.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- mm/shmem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/shmem.c b/mm/shmem.c index 2b33076c0f9c..82911fefc2d5 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2452,7 +2452,7 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, */ pagefault_disable(); ret = copy_from_user(page_kaddr, - uaddr_to_user_ptr_safe(src_addr), + (const void __user *)src_addr, PAGE_SIZE); pagefault_enable(); kunmap_local(page_kaddr);
audit_log_execve_info() wants to read the program's arguments directly from user memory; to perform the uaccess we therefore need to create a valid user pointer from the address of the arguments.
uaddr_to_user_ptr_safe() should no longer be used for that purpose. Instead, we use make_user_ptr_for_read_uaccess() to create a user pointer with appropriate bounds and permissions (in PCuABI).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/auditsc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c index a136fb44180c..d66ebe85eff8 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1129,7 +1129,8 @@ static void audit_log_execve_info(struct audit_context *context, unsigned int arg; char *buf_head; char *buf; - const char __user *p = uaddr_to_user_ptr_safe(current->mm->arg_start); + const char __user *p = make_user_ptr_for_read_uaccess( + current->mm->arg_start, MAX_EXECVE_AUDIT_LEN);
/* NOTE: this buffer needs to be large enough to hold all the non-arg * data we put in the audit record for this argument (see the
arch_perf_out_copy_user() allows copying arbitrary data from user memory.
arch_perf_out_copy_user() does not get passed a user pointer, so at the moment we need to create one. Because the address it gets passed is arbitrary, using uaddr_to_user_ptr_safe() is not appropriate. Use uaddr_to_user_ptr() instead, until we find a more permanent solution.
Currently, the only user of arch_perf_out_copy_user() is perf_output_sample_ustack(), which reads a certain amount of data from the user stack as sample for PERF_SAMPLE_STACK_USER. It should be possible to get this function to pass a full user pointer to the copy routine, though this creates some complications:
- DEFINE_OUTPUT_COPY() would need to handle either a kernel or user pointer (may need a new macro).
- perf_output_sample_ustack() would need to obtain the actual stack pointer, but perf_user_stack_pointer() currently returns an unsigned long. Most likely this means changing user_stack_pointer() to return a user_uintptr_t, with an appropriate implementation in PCuABI.
- Compat would need to be handled carefully, probably using compat_ptr() to create a valid user pointer.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/events/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/internal.h b/kernel/events/internal.h index a68cd0b99b04..d1a7c0030c11 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -199,7 +199,7 @@ arch_perf_out_copy_user(void *dst, const void *src, unsigned long n) unsigned long ret;
pagefault_disable(); - ret = __copy_from_user_inatomic(dst, uaddr_to_user_ptr_safe((ptraddr_t)src), n); + ret = __copy_from_user_inatomic(dst, uaddr_to_user_ptr((ptraddr_t)src), n); pagefault_enable();
return ret;
aarch32_break_handler() and call_undef_hook() want to read an instruction at the current PC. instruction_pointer() currently returns just an address (not PCC in PCuABI), so to perform the uaccess we need to create a valid user pointer out of it.
uaddr_to_user_ptr_safe() should no longer be used for that purpose. Instead, we use make_user_ptr_for_read_uaccess() to create a user pointer with appropriate bounds and permissions (in PCuABI).
We may want to change instruction_pointer() to return a full user pointer eventually, however it is unclear whether this makes sense for most use-cases. Since this particular access should be completely safe, using make_user_ptr_for_read_uaccess() to create a user pointer is deemed acceptable.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/kernel/debug-monitors.c | 3 ++- arch/arm64/kernel/traps.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 4239cd747217..9c44906ad517 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -344,7 +344,8 @@ int aarch32_break_handler(struct pt_regs *regs) u32 arm_instr; u16 thumb_instr; bool bp = false; - void __user *pc = uaddr_to_user_ptr_safe(instruction_pointer(regs)); + const void __user *pc = make_user_ptr_for_read_uaccess( + instruction_pointer(regs), sizeof(arm_instr));
if (!compat_user_mode(regs)) return -EFAULT; diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 5d164223a580..b50f657c0b21 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -401,7 +401,8 @@ static int call_undef_hook(struct pt_regs *regs) u32 instr; int (*fn)(struct pt_regs *regs, u32 instr) = NULL; unsigned long pc = instruction_pointer(regs); - void __user *pc_usr_ptr = uaddr_to_user_ptr_safe(pc); + const void __user *pc_usr_ptr = make_user_ptr_for_read_uaccess( + pc, sizeof(instr));
if (!user_mode(regs)) { __le32 instr_le;
Hi Kevin,
On 6/22/23 18:28, Kevin Brodsky wrote:
Hi,
This is a small update to the series, for more info see the original cover letter [1].
Following various discussions, the interface remains broadly unchanged, with a separate function for each permission set (read/write/RW). Luca's work on explicit checking however exposed that check_user_ptr_read() should clearly take a pointer to const. The write/RW variants still take a non-const pointer; accordingly fault_in_safe_writeable() will be changed to take a non-const pointer too [2], which feels more logical.
For symmetry, make_user_ptr_for_read_uaccess() now returns a pointer to const. A few patches are modified accordingly; the additional changes are minimal. A nice side-effect is that this reduces the risks of misuse at compile time, since copy_to_user(uptr, ...) will trigger a warning if uptr is a pointer to const.
v1..v2:
- Patch 1: made check_user_ptr_read() and make_user_ptr_for_read_uaccess() take/return a pointer to const.
- Patch 3, 4, 9: made variables pointers to const when using make_user_ptr_for_read_uaccess().
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/user_pt...
I had a look the series and overall they seem to be fine. make_user_ptr_for_[*]_uaccess several use cases mentioned here as well as in Luca's series justifies this new helpers.
Thanks, Amit
Thanks, Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Kevin Brodsky (9): linux/user_ptr.h: Introduce uaccess-related helpers fs/binfmt_elf: Create appropriate user pointer for uaccess coredump: Create appropriate user pointer for uaccess mm/memory: Create appropriate user pointer for uaccess Revert "mm/hugetlb: Use appropriate user pointer conversions" Revert "mm/shmem: Use appropriate user pointer conversions" audit: Create appropriate user pointer for uaccess perf: Avoid uaddr_to_user_ptr_safe() for arbitrary user address arm64: Create appropriate user pointer for uaccess
Documentation/core-api/user_ptr.rst | 100 ++++++++++++++++++---------- arch/arm64/kernel/debug-monitors.c | 3 +- arch/arm64/kernel/traps.c | 3 +- fs/binfmt_elf.c | 14 ++-- fs/coredump.c | 4 +- include/linux/user_ptr.h | 86 ++++++++++++++++++++++-- kernel/auditsc.c | 3 +- kernel/events/internal.h | 2 +- lib/user_ptr.c | 46 +++++++++++++ mm/hugetlb.c | 2 +- mm/memory.c | 4 +- mm/shmem.c | 2 +- 12 files changed, 218 insertions(+), 51 deletions(-)
On 28/06/2023 12:45, Amit Daniel Kachhap wrote:
Hi Kevin,
On 6/22/23 18:28, Kevin Brodsky wrote:
Hi,
This is a small update to the series, for more info see the original cover letter [1].
Following various discussions, the interface remains broadly unchanged, with a separate function for each permission set (read/write/RW). Luca's work on explicit checking however exposed that check_user_ptr_read() should clearly take a pointer to const. The write/RW variants still take a non-const pointer; accordingly fault_in_safe_writeable() will be changed to take a non-const pointer too [2], which feels more logical.
For symmetry, make_user_ptr_for_read_uaccess() now returns a pointer to const. A few patches are modified accordingly; the additional changes are minimal. A nice side-effect is that this reduces the risks of misuse at compile time, since copy_to_user(uptr, ...) will trigger a warning if uptr is a pointer to const.
v1..v2:
- Patch 1: made check_user_ptr_read() and
make_user_ptr_for_read_uaccess() take/return a pointer to const.
- Patch 3, 4, 9: made variables pointers to const when using
make_user_ptr_for_read_uaccess().
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/user_pt...
I had a look the series and overall they seem to be fine. make_user_ptr_for_[*]_uaccess several use cases mentioned here as well as in Luca's series justifies this new helpers.
Thank you for having another look! There seems to be no concern with the current state of the series, so I have now applied it on next.
Kevin
Thanks, Amit
Thanks, Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... [2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Kevin Brodsky (9): linux/user_ptr.h: Introduce uaccess-related helpers fs/binfmt_elf: Create appropriate user pointer for uaccess coredump: Create appropriate user pointer for uaccess mm/memory: Create appropriate user pointer for uaccess Revert "mm/hugetlb: Use appropriate user pointer conversions" Revert "mm/shmem: Use appropriate user pointer conversions" audit: Create appropriate user pointer for uaccess perf: Avoid uaddr_to_user_ptr_safe() for arbitrary user address arm64: Create appropriate user pointer for uaccess
Documentation/core-api/user_ptr.rst | 100 ++++++++++++++++++---------- arch/arm64/kernel/debug-monitors.c | 3 +- arch/arm64/kernel/traps.c | 3 +- fs/binfmt_elf.c | 14 ++-- fs/coredump.c | 4 +- include/linux/user_ptr.h | 86 ++++++++++++++++++++++-- kernel/auditsc.c | 3 +- kernel/events/internal.h | 2 +- lib/user_ptr.c | 46 +++++++++++++ mm/hugetlb.c | 2 +- mm/memory.c | 4 +- mm/shmem.c | 2 +- 12 files changed, 218 insertions(+), 51 deletions(-)
linux-morello@op-lists.linaro.org