- linux-morello - op-lists.linaro.org

[PATCH v2 0/8] Add explicit capability checking

by Luca Vizzarro

Hello! Finally posting the second version of my patch series relating the explicit capability checking[1]. After some deep investigation carried out together with Kevin and Tudor – thank you both for your help! –, we may be getting closer to the end of this unexpectedly laborious task. As per before, this series is dependent on Kevin's user pointer checking helper functions and a WIP implementation of PCuABI for the futex_waitv syscall so that the kernel doesn't break. All of this work is available together for testing – it's been successfully tested with LTP by myself – on my fork's branch[2]. Finally, it now includes a patch by Al Viro recently merged on mainline Linux. This aims at rendering the implementation of iov_iter less confusing, and actually aided the investigation of this task. Kind regards, Luca [1] https://git.morello-project.org/morello/kernel/linux/-/issues/7 [2] https://git.morello-project.org/Sevenarth/linux/-/commits/morello/gup-check… Al Viro (1): use less confusing names for iov_iter direction initializers Luca Vizzarro (7): gup: Add explicit capability checks iov_iter: Add explicit capability checks bpf: Add explicit capability checks usb: core: Add explicit capability checks futex: Add explicit capability checks io_uring: Add explicit capability checks nvme: Add TODO for PCuABI implementation arch/s390/mm/maccess.c | 3 +-- arch/x86/kernel/cpu/microcode/intel.c | 2 +- arch/x86/kernel/crash_dump_64.c | 2 +- crypto/testmgr.c | 4 ++-- drivers/acpi/pfr_update.c | 2 +- drivers/block/drbd/drbd_main.c | 2 +- drivers/block/drbd/drbd_receiver.c | 2 +- drivers/block/loop.c | 12 ++++++------ drivers/block/nbd.c | 10 +++++----- drivers/char/random.c | 4 ++-- drivers/fsi/fsi-sbefifo.c | 6 +++--- drivers/infiniband/ulp/rtrs/rtrs-clt.c | 2 +- drivers/isdn/mISDN/l1oip_core.c | 2 +- drivers/misc/vmw_vmci/vmci_queue_pair.c | 6 +++--- drivers/net/ppp/ppp_generic.c | 2 +- drivers/nvme/host/ioctl.c | 1 + drivers/nvme/host/tcp.c | 4 ++-- drivers/nvme/target/io-cmd-file.c | 4 ++-- drivers/nvme/target/tcp.c | 2 +- drivers/s390/char/zcore.c | 2 +- drivers/scsi/sg.c | 2 +- drivers/target/iscsi/iscsi_target_util.c | 4 ++-- drivers/target/target_core_file.c | 2 +- drivers/usb/core/devio.c | 4 +++- drivers/usb/usbip/usbip_common.c | 2 +- drivers/vhost/net.c | 6 +++--- drivers/vhost/scsi.c | 10 +++++----- drivers/vhost/vhost.c | 6 +++--- drivers/vhost/vringh.c | 4 ++-- drivers/vhost/vsock.c | 4 ++-- drivers/xen/pvcalls-back.c | 8 ++++---- fs/9p/vfs_addr.c | 4 ++-- fs/9p/vfs_dir.c | 2 +- fs/9p/xattr.c | 4 ++-- fs/afs/cmservice.c | 2 +- fs/afs/dir.c | 2 +- fs/afs/file.c | 4 ++-- fs/afs/internal.h | 4 ++-- fs/afs/rxrpc.c | 10 +++++----- fs/afs/write.c | 4 ++-- fs/aio.c | 4 ++-- fs/btrfs/ioctl.c | 4 ++-- fs/ceph/addr.c | 4 ++-- fs/ceph/file.c | 4 ++-- fs/cifs/connect.c | 6 +++--- fs/cifs/file.c | 4 ++-- fs/cifs/fscache.c | 4 ++-- fs/cifs/smb2ops.c | 4 ++-- fs/cifs/transport.c | 6 +++--- fs/coredump.c | 2 +- fs/erofs/fscache.c | 6 +++--- fs/fscache/io.c | 2 +- fs/fuse/ioctl.c | 4 ++-- fs/netfs/io.c | 6 +++--- fs/nfs/fscache.c | 4 ++-- fs/nfsd/vfs.c | 4 ++-- fs/ocfs2/cluster/tcp.c | 2 +- fs/orangefs/inode.c | 8 ++++---- fs/proc/vmcore.c | 6 +++--- fs/read_write.c | 12 ++++++------ fs/seq_file.c | 2 +- fs/splice.c | 10 +++++----- include/linux/io_uring.h | 2 +- include/linux/pagemap.h | 2 +- include/linux/uio.h | 3 +++ io_uring/kbuf.c | 10 +++++++++- io_uring/net.c | 17 ++++++++--------- io_uring/rsrc.c | 17 ++++++++++++++--- io_uring/rsrc.h | 4 ++-- io_uring/rw.c | 12 ++++++------ io_uring/uring_cmd.c | 6 +++++- kernel/bpf/helpers.c | 4 +++- kernel/futex/core.c | 17 ++++++++++++++--- kernel/trace/trace_events_user.c | 2 +- lib/iov_iter.c | 21 ++++++++++++++++++--- mm/gup.c | 10 +++++++--- mm/madvise.c | 2 +- mm/page_io.c | 4 ++-- mm/process_vm_access.c | 2 +- net/9p/client.c | 2 +- net/bluetooth/6lowpan.c | 2 +- net/bluetooth/a2mp.c | 2 +- net/bluetooth/smp.c | 2 +- net/ceph/messenger_v1.c | 4 ++-- net/ceph/messenger_v2.c | 14 +++++++------- net/compat.c | 3 ++- net/ipv4/tcp.c | 4 ++-- net/netfilter/ipvs/ip_vs_sync.c | 2 +- net/smc/smc_clc.c | 6 +++--- net/smc/smc_tx.c | 2 +- net/socket.c | 12 ++++++------ net/sunrpc/socklib.c | 6 +++--- net/sunrpc/svcsock.c | 4 ++-- net/sunrpc/xprtsock.c | 6 +++--- net/tipc/topsrv.c | 2 +- net/tls/tls_device.c | 4 ++-- net/xfrm/espintcp.c | 2 +- security/keys/keyctl.c | 4 ++-- 98 files changed, 274 insertions(+), 214 deletions(-) -- 2.34.1

1 year, 5 months

3
15
0 0

[PATCH 0/9] New user_ptr helpers for uaccess

by Kevin Brodsky

Hi, This series introduces new user_ptr helpers to help in certain uaccess-related situations. This is a follow-up to my previous series "New CHERI API and separation of root capabilities"; the CHERI helpers it introduced are used to implement the new generic user_ptr helpers in PCuABI. The new helpers are (see patch 1 for details): * make_user_ptr_for_<perms>_uaccess(), to create user pointers in order to perform uaccess, with appropriate bounds and permissions. * check_user_ptr_<perms>(), to perform explicit checking of user pointers. This series does not actually make use of check_user_ptr_<perms>(), rather it prepares the ground for implementing explicit checking when user memory is accessed via kernel mappings [1]. The rest of the series (patch 2-9) is about converting existing uses of uaddr_to_user_ptr_safe(), as it should now only be used for *providing* user pointers to userspace, and not for uaccess. After this series, the only remaining users of uaddr_to_user_ptr_safe() are: - fs/binfmt_elf.c to provide all the initial capabilities (stack, AT_CHERI_*_CAP, etc.). uaddr_to_user_ptr_safe() is still used to write the initial data on the stack too; it didn't seem worthwhile to refactor this code as it is going to change anyway as part of [2] and [3]. - mmap / mremap / shmat to return a valid capability. To clarify which helper should be used in which situation, here are two tables specifying the helper to use depending on whether the address is specified by userspace or the kernel itself, and whether the pointer is provided to userspace or used by the kernel itself. *Before* this series: +-----------------------------------+---------------------+--------------------------+ | Pointer for \ Address provided by | User | Kernel | +===================================+=====================+==========================+ | User | - | uaddr_to_user_ptr_safe() | +-----------------------------------+---------------------+--------------------------+ | Kernel (uaccess) | uaddr_to_user_ptr() | uaddr_to_user_ptr_safe() | +-----------------------------------+---------------------+--------------------------+ *After* this series: +-----------------------------------+---------------------+-------------------------------+ | Pointer for \ Address provided by | User | Kernel | +===================================+=====================+===============================+ | User | - | uaddr_to_user_ptr_safe() | +-----------------------------------+---------------------+-------------------------------+ | Kernel (uaccess) | uaddr_to_user_ptr() | make_user_ptr_*_for_uaccess() | +-----------------------------------+---------------------+-------------------------------+ Eventually both uaddr_to_user_ptr() and uaddr_to_user_ptr_safe() should disappear, the first thanks to userspace always providing full pointers and the second being replaced by handcrafted code creating capabilities in line with the PCuABI spec (whose bounds give access to only the intended object and potentially padding). Note that patch 1 and 4 were included in the first RFC of the CHERI API series [4]. They remain broadly the same, but: - make_privileged_user_ptr and check_user_ptr() have been renamed, and the permissions are now specified by calling the right variant of the function instead of passing a bitfield. They are now called respectively make_user_ptr_for_<perms>_uaccess() and check_user_ptr_<perms>(). - The user_ptr documentation has been updated accordingly. - The commit messages have been improved to reflect the overall intention better. Review branch: https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/user_p… Rendered doc: https://git.morello-project.org/kbrodsky-arm/linux/-/blob/morello/user_ptr_… Thanks, Kevin [1] https://git.morello-project.org/morello/kernel/linux/-/issues/7 [2] https://git.morello-project.org/morello/kernel/linux/-/issues/19 [3] https://git.morello-project.org/morello/kernel/linux/-/issues/22 [4] 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 | 2 +- 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 | 2 +- mm/shmem.c | 2 +- 12 files changed, 216 insertions(+), 50 deletions(-) -- 2.38.1

1 year, 5 months

4
19
0 0

[PATCH] gup: Add explicit capability checks

by Luca Vizzarro

Whenever a GUP call is made, the page address for the lookup is a 64-bit long raw pointer. When working in PCuABI, this means that the metadata of the capability gets discarded, hence any access made by the GUP is not checked in hardware. This commit introduces explicit capability checks whenever a call to the current mm through the GUP functions is made. Signed-off-by: Luca Vizzarro <Luca.Vizzarro(a)arm.com> --- Hello, this patch adds explicit capability checks needed in conjuction with GUP calls. Submitting only for review and not merging. In order for this patch to work Kevin's user pointer helpers patch is required, in addition to a modification that I have suggested in his series thread. Another essential required change is actually porting the futex_waitv syscall to PCuABI. Otherwise this patch will be a BREAKING change and LTP will fail. LTP was ran successfully against this patch ONLY with the minimum required changes for futex_waitv to work, which are still a WIP. The ticket related to this patch is #7: https://git.morello-project.org/morello/kernel/linux/-/issues/7 This patch can be found on the following branch, which contains some futex_waitv changes, Kevin's series and its modification: https://git.morello-project.org/Sevenarth/linux/-/commits/morello/gup-checks Best, Luca --- drivers/usb/core/devio.c | 8 ++++++-- io_uring/kbuf.c | 5 ++++- io_uring/net.c | 4 +++- io_uring/rsrc.c | 7 ++++++- kernel/bpf/helpers.c | 4 +++- kernel/futex/core.c | 11 ++++++++--- lib/iov_iter.c | 21 ++++++++++++++++++--- mm/gup.c | 8 ++++++-- 8 files changed, 54 insertions(+), 14 deletions(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index cb37f6d2010a..4d3249ba343a 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1584,8 +1584,12 @@ find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb) { struct usb_memory *usbm = NULL, *iter; unsigned long flags; - /* TODO [PCuABI] - capability checks for uaccess */ - unsigned long uurb_start = user_ptr_addr(uurb->buffer); + unsigned long uurb_start; + + if (!check_user_ptr_rw(uurb->buffer, uurb->buffer_length)) + return ERR_PTR(-EFAULT); + + uurb_start = user_ptr_addr(uurb->buffer); spin_lock_irqsave(&ps->lock, flags); list_for_each_entry(iter, &ps->memory_list, memlist) { diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 70056c27d778..d6e6227caab0 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -587,7 +587,10 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg) pages_size = io_in_compat64(ctx) ? size_mul(sizeof(struct compat_io_uring_buf), reg.ring_entries) : size_mul(sizeof(struct io_uring_buf), reg.ring_entries); - /* TODO [PCuABI] - capability checks for uaccess */ + + if (!check_user_ptr_rw((void __user *)reg.ring_addr, pages_size)) + return -EFAULT; + pages = io_pin_pages(reg.ring_addr, pages_size, &nr_pages); if (IS_ERR(pages)) { kfree(free_bl); diff --git a/io_uring/net.c b/io_uring/net.c index 6fd28a49b671..a8766d53cad8 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -1088,7 +1088,9 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags) return io_setup_async_addr(req, &__address, issue_flags); if (zc->flags & IORING_RECVSEND_FIXED_BUF) { - /* TODO [PCuABI] - capability checks for uaccess */ + if (!check_user_ptr_write(zc->buf, zc->len)) + return -EFAULT; + ret = io_import_fixed(WRITE, &msg.msg_iter, req->imu, user_ptr_addr(zc->buf), zc->len); if (unlikely(ret)) diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 9e716fef91d7..285938fcf119 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -1285,7 +1285,12 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, struct iovec *iov, return 0; ret = -ENOMEM; - /* TODO [PCuABI] - capability checks for uaccess */ + + if (!check_user_ptr_rw(iov->iov_base, iov->iov_len)) { + ret = -EFAULT; + goto done; + } + pages = io_pin_pages(user_ptr_addr(iov->iov_base), iov->iov_len, &nr_pages); if (IS_ERR(pages)) { diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index a8e76cf06da7..4db910f1758d 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -675,7 +675,9 @@ BPF_CALL_5(bpf_copy_from_user_task, void *, dst, u32, size, if (unlikely(!size)) return 0; - /* TODO [PCuABI] - capability checks for uaccess */ + if (!check_user_ptr_read(user_ptr, size)) + return -EFAULT; + ret = access_process_vm(tsk, user_ptr_addr(user_ptr), dst, size, 0); if (ret == size) return 0; diff --git a/kernel/futex/core.c b/kernel/futex/core.c index 9613080ccf0c..04289dc13b4a 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -226,8 +226,6 @@ int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key, struct address_space *mapping; int err, ro = 0; - /* TODO [PCuABI] - capability checks for uaccess */ - /* * The futex address must be "naturally" aligned. */ @@ -239,6 +237,12 @@ int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key, if (unlikely(!access_ok(uaddr, sizeof(u32)))) return -EFAULT; + if (rw == FUTEX_READ && !check_user_ptr_read(uaddr, sizeof(u32))) + return -EFAULT; + + if (rw == FUTEX_WRITE && !check_user_ptr_rw(uaddr, sizeof(u32))) + return -EFAULT; + if (unlikely(should_fail_futex(fshared))) return -EFAULT; @@ -413,7 +417,8 @@ int fault_in_user_writeable(u32 __user *uaddr) struct mm_struct *mm = current->mm; int ret; - /* TODO [PCuABI] - capability checks for uaccess */ + if (!check_user_ptr_rw(uaddr, PAGE_SIZE)) + return -EFAULT; mmap_read_lock(mm); ret = fixup_user_fault(mm, user_ptr_addr(uaddr), diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 2d74d8d00ad9..046915cc1562 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1481,10 +1481,16 @@ static unsigned long first_iovec_segment(const struct iov_iter *i, size_t *size) { size_t skip; long k; + bool is_rw = iov_iter_rw(i) != WRITE; + + if (iter_is_ubuf(i)) { + if (is_rw && !check_user_ptr_rw(i->ubuf, *size)) + return -EFAULT; + if (!is_rw && !check_user_ptr_read(i->ubuf, *size)) + return -EFAULT; - if (iter_is_ubuf(i)) - /* TODO [PCuABI] - capability checks for uaccess */ return user_ptr_addr(i->ubuf) + i->iov_offset; + } for (k = 0, skip = i->iov_offset; k < i->nr_segs; k++, skip = 0) { size_t len = i->iov[k].iov_len - skip; @@ -1493,7 +1499,13 @@ static unsigned long first_iovec_segment(const struct iov_iter *i, size_t *size) continue; if (*size > len) *size = len; - /* TODO [PCuABI] - capability checks for uaccess */ + if (is_rw && !check_user_ptr_rw(i->iov[k].iov_base + skip, + *size)) + return -EFAULT; + if (!is_rw && !check_user_ptr_read(i->iov[k].iov_base + skip, + *size)) + return -EFAULT; + return user_ptr_addr(i->iov[k].iov_base) + skip; } BUG(); // if it had been empty, we wouldn't get called @@ -1539,6 +1551,9 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i, gup_flags |= FOLL_NOFAULT; addr = first_iovec_segment(i, &maxsize); + if (IS_ERR_VALUE(addr)) + return addr; + *start = addr % PAGE_SIZE; addr &= PAGE_MASK; n = want_pages_array(pages, maxsize, *start, maxpages); diff --git a/mm/gup.c b/mm/gup.c index dc02749eaf9b..197459c58cd1 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1843,11 +1843,15 @@ EXPORT_SYMBOL(fault_in_subpage_writeable); */ size_t fault_in_safe_writeable(const char __user *uaddr, size_t size) { - /* TODO [PCuABI] - capability checks for uaccess */ - unsigned long start = user_ptr_addr(uaddr), end; + unsigned long start, end; struct mm_struct *mm = current->mm; bool unlocked = false; + if (!check_user_ptr_read(uaddr, size)) + return 0; + + start = user_ptr_addr(uaddr); + if (unlikely(size == 0)) return 0; end = PAGE_ALIGN(start + size); -- 2.34.1

1 year, 5 months

3
14
0 0

[Discussion] check_user_ptr interface

by Kevin Brodsky

Hi, For the sake of clarity, I am starting this new thread to discuss the interface we should adopt for the check_user_ptr interface, introduced by the "New user_ptr helpers for uaccess" series [1.1]. This function is meant to check that the specifier user pointer (capability in PCuABI) allows a given range to be accessed in a given way. The range is specified quite naturally by the address of the pointer + a separate size argument. As to the nature of the access (read and/or write), as suggested [2.2] during the review of the preliminary RFC, it is currently specified in the function name itself: check_user_ptr_read(), check_user_ptr_write(), check_user_ptr_rw(). I believe this is looking pretty good, but Luca's recent explicit check patch [3] has shown that it is a little inconvenient in the cases where the nature of the access is only known at runtime, often through an integer set to either READ or WRITE. This led me to suggest [1.2] to use these constants as an argument for check_user_ptr too, only to realise later that it doesn't work, as it is not possible to use them together (READ | WRITE is meaningless). The question now is therefore the following: should we go back to something like in the RFC [2.1], i.e. pass the access type as argument, introducing a whole new type / constants for the access type? Or should we stick to [1.1], i.e. the access type in the function name itself. At this point I would tend to favour the latter option, as there are not so many situations where the access type is only known at runtime (just 3 so far), but maybe this is short-sighted. Opinions very welcome! Thanks, Kevin [1.1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org… [1.2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org… [2.1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org… [2.2] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org… [3] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org…

1 year, 5 months

1
0
0 0

[PATCH v2] io_uring: Don't modify user_data values in the SQE addr2 field

by Tudor Cretu

Some io_uring operations' SQEs store user_data values in the addr2 field. These don't need to be modified as they're not dereferenced by the kernel. Reported-by: Kevin Brodsky <kevin.brodsky(a)arm.com> Signed-off-by: Tudor Cretu <tudor.cretu(a)arm.com> --- v2: - Updated the comment that it's only propagated, not matched. Review branch: https://git.morello-project.org/tudcre01/linux/-/commits/morello/addr2_fix_… --- io_uring/io_uring.h | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index 5b4f0f298ad9..26cfe280b049 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -132,9 +132,22 @@ static inline void convert_compat64_io_uring_sqe(struct io_ring_ctx *ctx, sqe->ioprio = READ_ONCE(compat_sqe->ioprio); sqe->fd = READ_ONCE(compat_sqe->fd); BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr2, addr); - sqe->addr2 = (__kernel_uintptr_t)compat_ptr(READ_ONCE(compat_sqe->addr2)); - BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr, len); + /* + * Some opcodes set a user_data value in the addr2 field to propagate + * it as-is to the user_data field of a CQE. It's not dereferenced + * by the kernel, so don't modify it. + */ + switch (sqe->opcode) { + case IORING_OP_POLL_REMOVE: + case IORING_OP_MSG_RING: + sqe->addr2 = (__kernel_uintptr_t)READ_ONCE(compat_sqe->addr2); + break; + default: + sqe->addr2 = (__kernel_uintptr_t)compat_ptr(READ_ONCE(compat_sqe->addr2)); + break; + } + BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr, len); /* * Some opcodes set a user_data value in the addr field to be matched * with a pre-existing IO event's user_data. It's not dereferenced by -- 2.34.1

1 year, 6 months

2
1
0 0

[linux-morello][PATCH v4] proc: change proc_ops.proc_ioct handler signature

by Pawel Zalewski

The module net:sunrpc:cache.c will effectively use the same callback cache_ioctl for pointers passed from file_operations.unlocked_ioctl and proc_ops.proc_ioctl and it will expect pointers as the argument. Thus to be able to pass CHERI capabilities, the .proc_ioctl fp's arg input variable should also be of user_uintptr_t type. The proc_compat_ioctl handler in the pci/proc.c module is replaced with compat_noptr_ioctl as the arg is not being passed as a pointer into the proc_bus_pci_ioctl handler. Signed-off-by: Pawel Zalewski <pzalewski(a)thegoodpenguin.co.uk> --- drivers/pci/proc.c | 4 ++-- include/linux/proc_fs.h | 2 +- net/sunrpc/cache.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c index f967709082d6..e25a676fa25f 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -192,7 +192,7 @@ struct pci_filp_private { #endif /* HAVE_PCI_MMAP */ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct pci_dev *dev = pde_data(file_inode(file)); #ifdef HAVE_PCI_MMAP @@ -322,7 +322,7 @@ static const struct proc_ops proc_bus_pci_ops = { .proc_write = proc_bus_pci_write, .proc_ioctl = proc_bus_pci_ioctl, #ifdef CONFIG_COMPAT - .proc_compat_ioctl = proc_bus_pci_ioctl, + .proc_compat_ioctl = compat_noptr_ioctl, #endif #ifdef HAVE_PCI_MMAP .proc_open = proc_bus_pci_open, diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index 81d6e4ec2294..11d6c3620175 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -36,7 +36,7 @@ struct proc_ops { loff_t (*proc_lseek)(struct file *, loff_t, int); int (*proc_release)(struct inode *, struct file *); __poll_t (*proc_poll)(struct file *, struct poll_table_struct *); - long (*proc_ioctl)(struct file *, unsigned int, unsigned long); + long (*proc_ioctl)(struct file *, unsigned int, user_uintptr_t); #ifdef CONFIG_COMPAT long (*proc_compat_ioctl)(struct file *, unsigned int, unsigned long); #endif diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index c4aaef5430f1..b682aa223bd6 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1573,7 +1573,7 @@ static __poll_t cache_poll_procfs(struct file *filp, poll_table *wait) } static long cache_ioctl_procfs(struct file *filp, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { struct inode *inode = file_inode(filp); struct cache_detail *cd = pde_data(inode); -- 2.34.1

1 year, 6 months

1
0
0 0

[PATCH] [WIP]binfmt_elf: Restrict CHERI auxv permissions

by Teo Couprie Diaz

Retrict the permisions of the CHERI specific auxv members as defined in the PCuABI specification. Signed-off-by: Teo Couprie Diaz <teo.coupriediaz(a)arm.com> --- This is not complete by any mean and could be used as a starting point. This does work for restricting the permissions but I couldn't manage to restrict the bounds in a similar fashion. fs/binfmt_elf.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 1d82465cb9e9..96b4e1794c6c 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -308,21 +308,29 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, } #if defined(CONFIG_CHERI_PURECAP_UABI) && (ELF_COMPAT == 0) /* - * TODO [PCuABI] - Restrict bounds/perms for AT_CHERI_* entries + * TODO [PCuABI] - Restrict bounds for AT_CHERI_* entries */ NEW_AUX_ENT(AT_CHERI_EXEC_RW_CAP, (exec_load_info->start_elf_rw != ~0UL ? - elf_uaddr_to_user_ptr(exec_load_info->start_elf_rw) : + cheri_perms_and(elf_uaddr_to_user_ptr(exec_load_info->start_elf_rw), + (CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | CHERI_PERMS_WRITE | + ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR)) : NULL)); NEW_AUX_ENT(AT_CHERI_EXEC_RX_CAP, - elf_uaddr_to_user_ptr(exec_load_info->start_elf_rx)); + cheri_perms_and(elf_uaddr_to_user_ptr(exec_load_info->start_elf_rx), + (CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | CHERI_PERMS_EXEC | + ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR))); NEW_AUX_ENT(AT_CHERI_INTERP_RW_CAP, ((interp_load_addr && interp_load_info->start_elf_rw != ~0UL) ? - elf_uaddr_to_user_ptr(interp_load_info->start_elf_rw) : + cheri_perms_and(elf_uaddr_to_user_ptr(interp_load_info->start_elf_rw), + (CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | CHERI_PERMS_WRITE | + ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR)) : NULL)); NEW_AUX_ENT(AT_CHERI_INTERP_RX_CAP, (interp_load_addr ? - elf_uaddr_to_user_ptr(interp_load_info->start_elf_rx) : + cheri_perms_and(elf_uaddr_to_user_ptr(interp_load_info->start_elf_rx), + (CHERI_PERMS_ROOTCAP | CHERI_PERMS_READ | CHERI_PERMS_EXEC | + ARM_CAP_PERMISSION_BRANCH_SEALED_PAIR)) : NULL)); NEW_AUX_ENT(AT_CHERI_STACK_CAP, elf_uaddr_to_user_ptr(0)); NEW_AUX_ENT(AT_CHERI_SEAL_CAP, cheri_user_root_seal_cap); -- 2.34.1

1 year, 6 months

2
1
0 0

[PATCH 0/1] fs: Handle exact bounds for argv and envp

by Teo Couprie Diaz

With this patch capabilities provided to userspace to access argv and envp are properly bounded with the permissions defined in the PCuABI spec. This change is split between two files, as they handle two parts of the process. This cover letter tries to explain the process surrounding the argument strings, hopefully to facilitate reviewing the patch but also to check my understanding, and the logic implemented by the patch. Hopefully I can find some time to refine it after reviews, but in case I cannot I hope the details will be useful. # General patch comments One thing that might be an issue is that I don't see how to update fs/exec.c:bprm_stack_limits to handle this new process short of reproducing it almost entirely inside the function. There are warnings because elf_stack_put_user_cap expects uintcap_t and cheri_build_user_cap builds a capability, but I didn't really know how to handle that. The function itself might be entirely unecessary. checkpatch complains about elf_stack_put_user_cap, but it follows the style of the other functions, and from missing blank lines which appear to be there, so I'm not too sure what to do about it. # Giving argument strings to userspace Calling a new executable is done through execve, where userspace passes the argv and envp pointer arrays to the kernel. In fs/exec.c:do_execveat_common, the kernel creates a struct linux_binprm which is used throughout the process of loading a new binary. Among other things, it keeps track of the stack for the new executable, both the memory space and its current top, which it allocates here. The argument are copied to this new stack in fs/exec.c:copy_strings. It does so from the top of the stack, starting with the last envp element and ending with the first of argv: opposite to how it will be read later. The function allocates pages as it goes, thus splitting the string in at most page-sized chunks, and handles offsets where the page was already partially used or the remainder of the string will not fill it. This only puts the /strings/ themselves on the stack. Later, in fs/binfmt_elf.c:create_elf_tables, the kernel puts the auxv, argv and envp arrays themselves – thus the pointers to the strings put on the stack earlier – on the stack. It does so in the order expected by userspace: from bottom to top of the strings, so from the first of argv to the last of envp. # Making it CHERI We thus have a potential issue: the moment where the strings themselves are allocated is different from the place where the capabilities are created, with limited ways to pass information in between. However, one piece of information is available in both cases: the length of the null-terminated string. This way, we can get the same representable length from the length of the string, which remains unchanged, while allocating the string *and* when creating the capability. Assuming we can properly align the strings in copy_strings, we don't need more information to properly create a capability with exact bounds in create_elf_tables. Thankfully, we can re-use most of the machinery in copy_strings to do so. If the representable length of a string differs from its real length, we need to get the mask needed for aligning it for exact bounds. Given this new length, we can compute the position where it would end up without alignment and ALIGN_DOWN() as we are going downards the stack. This gives the position where the string *must* start for exact capability bounds. Thus, all the padding will be after the string. However as we are going backwards, we need to put the padding first and the string second. The code already goes through the string chunk by chunk, handling smaller chunks at page and argument boundaries. We can re-use this by first going through the length of padding, and once this is done go through the string as normal. This guarantees the string will start at the properly aligned address with the appropriate amount of padding behind the previous allocations. We do need to handle this padding and to detect when strings might need proper handling for exact bounds. As there isn't padding normally, we can simply check if there is some. If there is, get the representable length of the string and use it for the exact capability bounds. As all the padding is after the strings, if there was padding needed for alignment the next argument will start in the padding. This is slow, but just go through the zeroes until we find the real start of the argument and go from there. This is not really properly done in this revision, it is a remnant of the first draft and as such will improperly flag arguments as needing adjustment after one that needed. This is known but should be simply fixable by moving this looping through the zeroes after allocating the capability. In fs/binfmt_elf.c:create_elf_tables argv and envp are handled exactly the same but in different loops. I removed the comments from the envp section for conciseness. # Testing To generate a big enough argument, you can use this dirty bash script: bigarg=""; count=0; while [ $(echo $bigarg | wc -c) -le 20000 ]; do bigarg=${bigarg}"==${count}==This_should_be_a_really_big_arg_string" count=$(($count + 1)) done It is a bit unnecessary and slow but it does allow easily checking that it starts and ends in the proper places. Without the patch, the kernel log should show a CPU fault if $bigarg is passed as an argument or in the environment. With the patch, it should work fine. Hopefully this is useful for reviewing, and correct in the first place ! Review branch: https://git.morello-project.org/Teo-CD/linux/-/tree/review-arg-str-resticte… Commit itself: https://git.morello-project.org/Teo-CD/linux/-/commit/a4d9fe880d098e9f9fe1e… Thanks very much in advance ! Téo Teo Couprie Diaz (1): fs: Handle exact bounds for argv and envp fs/binfmt_elf.c | 111 ++++++++++++++++++++++++++++++++++++++++++-- fs/exec.c | 66 ++++++++++++++++++++++++++ include/linux/elf.h | 4 ++ 3 files changed, 178 insertions(+), 3 deletions(-) -- 2.34.1

1 year, 6 months

2
2
0 0

[linux-morello][PATCH v2 0/3] enable nfs rootfs

by Pawel Zalewski

This series of patches enables nfs rootfs support on the Morello board. Patch 01 is fixing the inital kernel build error associated with a wrong function pointer type within the sunrpc modules due to the unlocked_ioctl fp, the error occurs upon enabling nfs within the defconfig. Patch 02 deals with the fallout caused by changes inferred by patches 01. See details in the description of the patch. Patch 03 is enabling nfs rootfs by default in the kernel. It was confirmed that the kernel can boot with a nfs rootfs. V2 changes: - patch only the modules that are actually being used - change description and fix nits - address the incorrect proc_compat_ioctl Pawel Zalewski (3): net: sunrpc: fix unlocked_ioctl handler signature include: linux: fix proc_ioctl arm64: morello: enable nfs rootfs by default arch/arm64/configs/morello_transitional_pcuabi_defconfig | 2 ++ drivers/pci/proc.c | 4 ++-- include/linux/proc_fs.h | 2 +- net/sunrpc/cache.c | 6 +++--- net/sunrpc/rpc_pipe.c | 2 +- 5 files changed, 9 insertions(+), 7 deletions(-) -- 2.34.1

1 year, 6 months

2
5
0 0

[PATCH] io_uring: Don't modify user_data values in the SQE addr2 field

by Tudor Cretu

Some io_uring operations' SQEs store user_data values in the addr2 field. These don't need to be modified as they're not dereferenced by the kernel. Reported-by: Kevin Brodsky <kevin.brodsky(a)arm.com> Signed-off-by: Tudor Cretu <tudor.cretu(a)arm.com> --- Review branch: https://git.morello-project.org/tudcre01/linux/-/commits/morello/addr2_fix --- io_uring/io_uring.h | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index 5b4f0f298ad9..db4f91cc64b2 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -132,9 +132,23 @@ static inline void convert_compat64_io_uring_sqe(struct io_ring_ctx *ctx, sqe->ioprio = READ_ONCE(compat_sqe->ioprio); sqe->fd = READ_ONCE(compat_sqe->fd); BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr2, addr); - sqe->addr2 = (__kernel_uintptr_t)compat_ptr(READ_ONCE(compat_sqe->addr2)); - BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr, len); + /* + * Some opcodes set a user_data value in the addr2 field to be matched + * with a pre-existing IO event's user_data or to propagate it to the + * user_data field of a CQE. It's not dereferenced by the kernel, so + * don't modify it. + */ + switch (sqe->opcode) { + case IORING_OP_POLL_REMOVE: + case IORING_OP_MSG_RING: + sqe->addr2 = (__kernel_uintptr_t)READ_ONCE(compat_sqe->addr2); + break; + default: + sqe->addr2 = (__kernel_uintptr_t)compat_ptr(READ_ONCE(compat_sqe->addr2)); + break; + } + BUILD_BUG_COMPAT_SQE_UNION_ELEM(addr, len); /* * Some opcodes set a user_data value in the addr field to be matched * with a pre-existing IO event's user_data. It's not dereferenced by -- 2.34.1

1 year, 6 months

2
1
0 0

2024

2023

2022

linux-morello