arm64 has a feature called Top Byte Ignore, which allows to embed pointer tags into the top byte of each pointer. Userspace programs (such as HWASan, a memory debugging tool [1]) might use this feature and pass tagged user pointers to the kernel through syscalls or other interfaces.
This patch makes a few of the kernel interfaces accept tagged user pointers. The kernel is already able to handle user faults with tagged pointers and has the untagged_addr macro, which this patchset reuses.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Changes in v6: - Added annotations for user pointer casts found by sparse. - Rebased onto 050cdc6c (4.19-rc1+).
Changes in v5: - Added 3 new patches that add untagging to places found with static analysis. - Rebased onto 44c929e1 (4.18-rc8).
Changes in v4: - Added a selftest for checking that passing tagged pointers to the kernel succeeds. - Rebased onto 81e97f013 (4.18-rc1+).
Changes in v3: - Rebased onto e5c51f30 (4.17-rc6+). - Added linux-arch@ to the list of recipients.
Changes in v2: - Rebased onto 2d618bdf (4.17-rc3+). - Removed excessive untagging in gup.c. - Removed untagging pointers returned from __uaccess_mask_ptr.
Changes in v1: - Rebased onto 4.17-rc1.
Changes in RFC v2: - Added "#ifndef untagged_addr..." fallback in linux/uaccess.h instead of defining it for each arch individually. - Updated Documentation/arm64/tagged-pointers.txt. - Dropped "mm, arm64: untag user addresses in memory syscalls". - Rebased onto 3eb2ce82 (4.16-rc7).
Andrey Konovalov (11): arm64: add type casts to untagged_addr macro uaccess: add untagged_addr definition for other arches arm64: untag user addresses in access_ok and __uaccess_mask_ptr mm, arm64: untag user addresses in mm/gup.c lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user arm64: untag user address in __do_user_fault fs, arm64: untag user address in copy_mount_options usb, arm64: untag user addresses in devio arm64: update Documentation/arm64/tagged-pointers.txt selftests, arm64: add a selftest for passing tagged pointers to kernel arm64: annotate user pointers casts detected by sparse
Documentation/arm64/tagged-pointers.txt | 5 +-- arch/arm64/include/asm/compat.h | 2 +- arch/arm64/include/asm/uaccess.h | 16 ++++++---- arch/arm64/kernel/perf_callchain.c | 4 +-- arch/arm64/kernel/signal.c | 16 +++++----- arch/arm64/kernel/signal32.c | 6 ++-- arch/arm64/mm/fault.c | 4 +-- block/compat_ioctl.c | 15 +++++---- drivers/ata/libata-scsi.c | 2 +- drivers/block/loop.c | 2 +- drivers/gpio/gpiolib.c | 8 +++-- drivers/input/evdev.c | 2 +- drivers/media/dvb-core/dvb_frontend.c | 3 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +++--- drivers/mmc/core/block.c | 6 ++-- drivers/mtd/mtdchar.c | 2 +- drivers/net/tap.c | 2 +- drivers/net/tun.c | 2 +- drivers/spi/spidev.c | 6 ++-- drivers/tty/tty_ioctl.c | 3 +- drivers/tty/vt/vt_ioctl.c | 5 +-- drivers/usb/core/devio.c | 10 ++++-- drivers/vfio/vfio.c | 6 ++-- drivers/video/fbdev/core/fbmem.c | 4 +-- drivers/xen/gntdev.c | 6 ++-- drivers/xen/privcmd.c | 4 +-- fs/aio.c | 2 +- fs/autofs/dev-ioctl.c | 3 +- fs/autofs/root.c | 2 +- fs/binfmt_elf.c | 10 +++--- fs/btrfs/ioctl.c | 2 +- fs/compat_ioctl.c | 32 ++++++++++--------- fs/ext2/ioctl.c | 2 +- fs/ext4/ioctl.c | 2 +- fs/fat/file.c | 3 +- fs/fuse/file.c | 2 +- fs/namespace.c | 2 +- fs/readdir.c | 4 +-- fs/signalfd.c | 10 +++--- include/linux/mm.h | 2 +- include/linux/pagemap.h | 8 ++--- include/linux/socket.h | 2 +- include/linux/uaccess.h | 4 +++ ipc/shm.c | 4 +-- kernel/futex.c | 6 ++-- kernel/futex_compat.c | 2 +- kernel/power/user.c | 2 +- kernel/signal.c | 2 +- lib/iov_iter.c | 16 +++++----- lib/strncpy_from_user.c | 4 ++- lib/strnlen_user.c | 6 ++-- lib/test_kasan.c | 2 +- mm/gup.c | 4 +++ mm/memory.c | 2 +- mm/migrate.c | 4 +-- mm/process_vm_access.c | 13 ++++---- net/bluetooth/hidp/sock.c | 2 +- net/compat.c | 12 ++++--- sound/core/control_compat.c | 5 +-- sound/core/pcm_native.c | 5 +-- sound/core/timer_compat.c | 3 +- tools/testing/selftests/arm64/.gitignore | 1 + tools/testing/selftests/arm64/Makefile | 11 +++++++ .../testing/selftests/arm64/run_tags_test.sh | 12 +++++++ tools/testing/selftests/arm64/tags_test.c | 19 +++++++++++ 65 files changed, 232 insertions(+), 147 deletions(-) create mode 100644 tools/testing/selftests/arm64/.gitignore create mode 100644 tools/testing/selftests/arm64/Makefile create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh create mode 100644 tools/testing/selftests/arm64/tags_test.c
This patch makes the untagged_addr macro accept all kinds of address types (void *, unsigned long, etc.) and allows not to specify type casts in each place where it is used. This is done by using __typeof__.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- arch/arm64/include/asm/uaccess.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index e66b0fca99c2..2d6451cbaa86 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -102,7 +102,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si * up with a tagged userland pointer. Clear the tag to get a sane pointer to * pass on to access_ok(), for instance. */ -#define untagged_addr(addr) sign_extend64(addr, 55) +#define untagged_addr(addr) \ + ((__typeof__(addr))sign_extend64((__u64)(addr), 55))
#define access_ok(type, addr, size) __range_ok(addr, size) #define user_addr_max get_fs
To allow arm64 syscalls accept tagged pointers from userspace, we must untag them when they are passed to the kernel. Since untagging is done in generic parts of the kernel, the untagged_addr macro needs to be defined for all architectures.
Define it as a noop for other architectures besides arm64.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- include/linux/uaccess.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index efe79c1cdd47..c045b4eff95e 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -13,6 +13,10 @@
#include <asm/uaccess.h>
+#ifndef untagged_addr +#define untagged_addr(addr) addr +#endif + /* * Architectures should provide two primitives (raw_copy_{to,from}_user()) * and get rid of their private instances of copy_{to,from}_user() and
copy_from_user (and a few other similar functions) are used to copy data from user memory into the kernel memory or vice versa. Since a user can provided a tagged pointer to one of the syscalls that use copy_from_user, we need to correctly handle such pointers.
Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr, before performing access validity checks.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- arch/arm64/include/asm/uaccess.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 2d6451cbaa86..fa7318d3d7d5 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -105,7 +105,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si #define untagged_addr(addr) \ ((__typeof__(addr))sign_extend64((__u64)(addr), 55))
-#define access_ok(type, addr, size) __range_ok(addr, size) +#define access_ok(type, addr, size) \ + __range_ok(untagged_addr(addr), size) #define user_addr_max get_fs
#define _ASM_EXTABLE(from, to) \ @@ -237,7 +238,8 @@ static inline void uaccess_enable_not_uao(void)
/* * Sanitise a uaccess pointer such that it becomes NULL if above the - * current addr_limit. + * current addr_limit. In case the pointer is tagged (has the top byte set), + * untag the pointer before checking. */ #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr) static inline void __user *__uaccess_mask_ptr(const void __user *ptr) @@ -245,10 +247,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) void __user *safe_ptr;
asm volatile( - " bics xzr, %1, %2\n" + " bics xzr, %3, %2\n" " csel %0, %1, xzr, eq\n" : "=&r" (safe_ptr) - : "r" (ptr), "r" (current_thread_info()->addr_limit) + : "r" (ptr), "r" (current_thread_info()->addr_limit), + "r" (untagged_addr(ptr)) : "cc");
csdb();
mm/gup.c provides a kernel interface that accepts user addresses and manipulates user pages directly (for example get_user_pages, that is used by the futex syscall). Since a user can provided tagged addresses, we need to handle such case.
Add untagging to gup.c functions that use user addresses for vma lookup.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- mm/gup.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/mm/gup.c b/mm/gup.c index 1abc8b4afff6..6f09132c654e 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -666,6 +666,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, if (!nr_pages) return 0;
+ start = untagged_addr(start); + VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
/* @@ -820,6 +822,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, struct vm_area_struct *vma; vm_fault_t ret, major = 0;
+ address = untagged_addr(address); + if (unlocked) fault_flags |= FAULT_FLAG_ALLOW_RETRY;
strncpy_from_user and strnlen_user accept user addresses as arguments, and do not go through the same path as copy_from_user and others, so here we need to handle the case of tagged user addresses separately.
Untag user pointers passed to these functions.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- lib/strncpy_from_user.c | 2 ++ lib/strnlen_user.c | 2 ++ 2 files changed, 4 insertions(+)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index b53e1b5d80f4..97467cd2bc59 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -106,6 +106,8 @@ long strncpy_from_user(char *dst, const char __user *src, long count) if (unlikely(count <= 0)) return 0;
+ src = untagged_addr(src); + max_addr = user_addr_max(); src_addr = (unsigned long)src; if (likely(src_addr < max_addr)) { diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c index 60d0bbda8f5e..8b5f56466e00 100644 --- a/lib/strnlen_user.c +++ b/lib/strnlen_user.c @@ -108,6 +108,8 @@ long strnlen_user(const char __user *str, long count) if (unlikely(count <= 0)) return 0;
+ str = untagged_addr(str); + max_addr = user_addr_max(); src_addr = (unsigned long)str; if (likely(src_addr < max_addr)) {
In __do_user_fault the fault address is being compared to TASK_SIZE to find out whether the address lies in the kernel or in user space. Since the fault address is coming from a user it can be tagged.
Untag the pointer before comparing.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- arch/arm64/mm/fault.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 50b30ff30de4..871fb3c38b23 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -313,7 +313,7 @@ static void __do_user_fault(struct siginfo *info, unsigned int esr) * type", so we ignore this wrinkle and just return the translation * fault.) */ - if (current->thread.fault_address >= TASK_SIZE) { + if (untagged_addr(current->thread.fault_address) >= TASK_SIZE) { switch (ESR_ELx_EC(esr)) { case ESR_ELx_EC_DABT_LOW: /*
In copy_mount_options a user address is being subtracted from TASK_SIZE. If the address is lower than TASK_SIZE, the size is calculated to not allow the exact_copy_from_user() call to cross TASK_SIZE boundary. However if the address is tagged, then the size will be calculated incorrectly.
Untag the address before subtracting.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- fs/namespace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/namespace.c b/fs/namespace.c index 99186556f8d3..51f763fb9430 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2672,7 +2672,7 @@ void *copy_mount_options(const void __user * data) * the remainder of the page. */ /* copy_from_user cannot cross TASK_SIZE ! */ - size = TASK_SIZE - (unsigned long)data; + size = TASK_SIZE - (unsigned long)untagged_addr(data); if (size > PAGE_SIZE) size = PAGE_SIZE;
devio allows to mmap memory regions and keeps them in a list. It also accepts a user address through an ioctl call and searches the memory region list for the region that contains this address. Since the addresses provided to mmap must not be tagged, and the addresses provided to ioctl might be tagged, we might compare tagged and untagged addresses during the search.
Untag the provided addresses before searching.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- drivers/usb/core/devio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 6ce77b33da61..ed5ab7c8100b 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1405,7 +1405,7 @@ find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb) { struct usb_memory *usbm = NULL, *iter; unsigned long flags; - unsigned long uurb_start = (unsigned long)uurb->buffer; + unsigned long uurb_start = (unsigned long)untagged_addr(uurb->buffer);
spin_lock_irqsave(&ps->lock, flags); list_for_each_entry(iter, &ps->memory_list, memlist) { @@ -1634,7 +1634,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb } } else if (uurb->buffer_length > 0) { if (as->usbm) { - unsigned long uurb_start = (unsigned long)uurb->buffer; + unsigned long uurb_start = + (unsigned long)untagged_addr(uurb->buffer);
as->urb->transfer_buffer = as->usbm->mem + (uurb_start - as->usbm->vm_start); @@ -1713,7 +1714,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb as->ps = ps; as->userurb = arg; if (as->usbm) { - unsigned long uurb_start = (unsigned long)uurb->buffer; + unsigned long uurb_start = + (unsigned long)untagged_addr(uurb->buffer);
as->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; as->urb->transfer_dma = as->usbm->dma_handle +
Add a note that work on passing tagged user pointers to the kernel via syscalls has started, but might not be complete yet.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- Documentation/arm64/tagged-pointers.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/arm64/tagged-pointers.txt b/Documentation/arm64/tagged-pointers.txt index a25a99e82bb1..361481283f00 100644 --- a/Documentation/arm64/tagged-pointers.txt +++ b/Documentation/arm64/tagged-pointers.txt @@ -35,8 +35,9 @@ Using non-zero address tags in any of these locations may result in an error code being returned, a (fatal) signal being raised, or other modes of failure.
-For these reasons, passing non-zero address tags to the kernel via -system calls is forbidden, and using a non-zero address tag for sp is +Some initial work for supporting non-zero address tags passed to the +kernel via system calls has been done, but the kernel doesn't provide +any guarantees at this point. Using a non-zero address tag for sp is strongly discouraged.
Programs maintaining a frame pointer and frame records that use non-zero
This patch adds a simple test, that calls the uname syscall with a tagged user pointer as an argument. Without the kernel accepting tagged user pointers the test fails with EFAULT.
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- tools/testing/selftests/arm64/.gitignore | 1 + tools/testing/selftests/arm64/Makefile | 11 +++++++++++ .../testing/selftests/arm64/run_tags_test.sh | 12 ++++++++++++ tools/testing/selftests/arm64/tags_test.c | 19 +++++++++++++++++++ 4 files changed, 43 insertions(+) create mode 100644 tools/testing/selftests/arm64/.gitignore create mode 100644 tools/testing/selftests/arm64/Makefile create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh create mode 100644 tools/testing/selftests/arm64/tags_test.c
diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore new file mode 100644 index 000000000000..e8fae8d61ed6 --- /dev/null +++ b/tools/testing/selftests/arm64/.gitignore @@ -0,0 +1 @@ +tags_test diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile new file mode 100644 index 000000000000..a61b2e743e99 --- /dev/null +++ b/tools/testing/selftests/arm64/Makefile @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0 + +# ARCH can be overridden by the user for cross compiling +ARCH ?= $(shell uname -m 2>/dev/null || echo not) + +ifneq (,$(filter $(ARCH),aarch64 arm64)) +TEST_GEN_PROGS := tags_test +TEST_PROGS := run_tags_test.sh +endif + +include ../lib.mk diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh new file mode 100755 index 000000000000..745f11379930 --- /dev/null +++ b/tools/testing/selftests/arm64/run_tags_test.sh @@ -0,0 +1,12 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +echo "--------------------" +echo "running tags test" +echo "--------------------" +./tags_test +if [ $? -ne 0 ]; then + echo "[FAIL]" +else + echo "[PASS]" +fi diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c new file mode 100644 index 000000000000..1452ed7d33f9 --- /dev/null +++ b/tools/testing/selftests/arm64/tags_test.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <stdio.h> +#include <unistd.h> +#include <stdint.h> +#include <sys/utsname.h> + +#define SHIFT_TAG(tag) ((uint64_t)(tag) << 56) +#define SET_TAG(ptr, tag) (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \ + SHIFT_TAG(tag)) + +int main(void) +{ + struct utsname utsname; + void *ptr = &utsname; + void *tagged_ptr = (void *)SET_TAG(ptr, 0x42); + int err = uname(tagged_ptr); + return err; +}
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Signed-off-by: Andrey Konovalov andreyknvl@google.com --- arch/arm64/include/asm/compat.h | 2 +- arch/arm64/include/asm/uaccess.h | 4 +-- arch/arm64/kernel/perf_callchain.c | 4 +-- arch/arm64/kernel/signal.c | 16 +++++----- arch/arm64/kernel/signal32.c | 6 ++-- arch/arm64/mm/fault.c | 2 +- block/compat_ioctl.c | 15 +++++---- drivers/ata/libata-scsi.c | 2 +- drivers/block/loop.c | 2 +- drivers/gpio/gpiolib.c | 8 +++-- drivers/input/evdev.c | 2 +- drivers/media/dvb-core/dvb_frontend.c | 3 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +++--- drivers/mmc/core/block.c | 6 ++-- drivers/mtd/mtdchar.c | 2 +- drivers/net/tap.c | 2 +- drivers/net/tun.c | 2 +- drivers/spi/spidev.c | 6 ++-- drivers/tty/tty_ioctl.c | 3 +- drivers/tty/vt/vt_ioctl.c | 5 +-- drivers/usb/core/devio.c | 8 +++-- drivers/vfio/vfio.c | 6 ++-- drivers/video/fbdev/core/fbmem.c | 4 +-- drivers/xen/gntdev.c | 6 ++-- drivers/xen/privcmd.c | 4 +-- fs/aio.c | 2 +- fs/autofs/dev-ioctl.c | 3 +- fs/autofs/root.c | 2 +- fs/binfmt_elf.c | 10 +++--- fs/btrfs/ioctl.c | 2 +- fs/compat_ioctl.c | 32 ++++++++++--------- fs/ext2/ioctl.c | 2 +- fs/ext4/ioctl.c | 2 +- fs/fat/file.c | 3 +- fs/fuse/file.c | 2 +- fs/namespace.c | 2 +- fs/readdir.c | 4 +-- fs/signalfd.c | 10 +++--- include/linux/mm.h | 2 +- include/linux/pagemap.h | 8 ++--- include/linux/socket.h | 2 +- ipc/shm.c | 4 +-- kernel/futex.c | 6 ++-- kernel/futex_compat.c | 2 +- kernel/power/user.c | 2 +- kernel/signal.c | 2 +- lib/iov_iter.c | 16 +++++----- lib/strncpy_from_user.c | 2 +- lib/strnlen_user.c | 4 +-- lib/test_kasan.c | 2 +- mm/memory.c | 2 +- mm/migrate.c | 4 +-- mm/process_vm_access.c | 13 ++++---- net/bluetooth/hidp/sock.c | 2 +- net/compat.c | 12 ++++--- sound/core/control_compat.c | 5 +-- sound/core/pcm_native.c | 5 +-- sound/core/timer_compat.c | 3 +- 58 files changed, 163 insertions(+), 140 deletions(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 1a037b94eba1..66e023fcea0a 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -155,7 +155,7 @@ static inline void __user *compat_ptr(compat_uptr_t uptr)
static inline compat_uptr_t ptr_to_compat(void __user *uptr) { - return (u32)(unsigned long)uptr; + return (u32)(__force unsigned long)uptr; }
#define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current))) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index fa7318d3d7d5..9b22c0be5c0b 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -76,7 +76,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si { unsigned long ret, limit = current_thread_info()->addr_limit;
- __chk_user_ptr(addr); + __chk_user_ptr((void __force *)addr); asm volatile( // A + B <= C + 1 for all A,B,C, in four easy steps: // 1: X = A + B; X' = X % 2^64 @@ -103,7 +103,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si * pass on to access_ok(), for instance. */ #define untagged_addr(addr) \ - ((__typeof__(addr))sign_extend64((__u64)(addr), 55)) + ((__typeof__(addr))sign_extend64((__force __u64)(addr), 55))
#define access_ok(type, addr, size) \ __range_ok(untagged_addr(addr), size) diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c index bcafd7dcfe8b..e2d781b9e7ea 100644 --- a/arch/arm64/kernel/perf_callchain.c +++ b/arch/arm64/kernel/perf_callchain.c @@ -123,7 +123,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry, tail = (struct frame_tail __user *)regs->regs[29];
while (entry->nr < entry->max_stack && - tail && !((unsigned long)tail & 0xf)) + tail && !((__force unsigned long)tail & 0xf)) tail = user_backtrace(tail, entry); } else { #ifdef CONFIG_COMPAT @@ -133,7 +133,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry, tail = (struct compat_frame_tail __user *)regs->compat_fp - 1;
while ((entry->nr < entry->max_stack) && - tail && !((unsigned long)tail & 0x3)) + tail && !((__force unsigned long)tail & 0x3)) tail = compat_user_backtrace(tail, entry); #endif } diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 5dcc942906db..da67d0bd1628 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -351,7 +351,7 @@ static int parse_user_sigframe(struct user_ctxs *user, user->fpsimd = NULL; user->sve = NULL;
- if (!IS_ALIGNED((unsigned long)base, 16)) + if (!IS_ALIGNED((__force unsigned long)base, 16)) goto invalid;
while (1) { @@ -450,7 +450,7 @@ static int parse_user_sigframe(struct user_ctxs *user, have_extra_context = true;
base = (__force void __user *)extra_datap; - if (!IS_ALIGNED((unsigned long)base, 16)) + if (!IS_ALIGNED((__force unsigned long)base, 16)) goto invalid;
if (!IS_ALIGNED(extra_size, 16)) @@ -742,16 +742,16 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka, __sigrestore_t sigtramp;
regs->regs[0] = usig; - regs->sp = (unsigned long)user->sigframe; - regs->regs[29] = (unsigned long)&user->next_frame->fp; - regs->pc = (unsigned long)ka->sa.sa_handler; + regs->sp = (__force unsigned long)user->sigframe; + regs->regs[29] = (__force unsigned long)&user->next_frame->fp; + regs->pc = (__force unsigned long)ka->sa.sa_handler;
if (ka->sa.sa_flags & SA_RESTORER) sigtramp = ka->sa.sa_restorer; else sigtramp = VDSO_SYMBOL(current->mm->context.vdso, sigtramp);
- regs->regs[30] = (unsigned long)sigtramp; + regs->regs[30] = (__force unsigned long)sigtramp; }
static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, @@ -777,8 +777,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, setup_return(regs, &ksig->ka, &user, usig); if (ksig->ka.sa.sa_flags & SA_SIGINFO) { err |= copy_siginfo_to_user(&frame->info, &ksig->info); - regs->regs[1] = (unsigned long)&frame->info; - regs->regs[2] = (unsigned long)&frame->uc; + regs->regs[1] = (__force unsigned long)&frame->info; + regs->regs[2] = (__force unsigned long)&frame->uc; } }
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 24b09003f821..184178a552d6 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -483,8 +483,10 @@ int compat_setup_rt_frame(int usig, struct ksignal *ksig,
if (err == 0) { compat_setup_return(regs, &ksig->ka, frame->sig.retcode, frame, usig); - regs->regs[1] = (compat_ulong_t)(unsigned long)&frame->info; - regs->regs[2] = (compat_ulong_t)(unsigned long)&frame->sig.uc; + regs->regs[1] = + (compat_ulong_t)(__force unsigned long)&frame->info; + regs->regs[2] = + (compat_ulong_t)(__force unsigned long)&frame->sig.uc; }
return err; diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 871fb3c38b23..0978b838f46e 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -299,7 +299,7 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
static void __do_user_fault(struct siginfo *info, unsigned int esr) { - current->thread.fault_address = (unsigned long)info->si_addr; + current->thread.fault_address = (__force unsigned long)info->si_addr;
/* * If the faulting address is in the kernel, we must sanitize the ESR. diff --git a/block/compat_ioctl.c b/block/compat_ioctl.c index 6ca015f92766..35aa46e0e289 100644 --- a/block/compat_ioctl.c +++ b/block/compat_ioctl.c @@ -85,7 +85,7 @@ static int compat_hdio_ioctl(struct block_device *bdev, fmode_t mode,
p = compat_alloc_user_space(sizeof(unsigned long)); error = __blkdev_driver_ioctl(bdev, mode, - cmd, (unsigned long)p); + cmd, (__force unsigned long)p); if (error == 0) { unsigned int __user *uvp = compat_ptr(arg); unsigned long v; @@ -138,7 +138,7 @@ static int compat_cdrom_read_audio(struct block_device *bdev, fmode_t mode, return -EFAULT;
return __blkdev_driver_ioctl(bdev, mode, cmd, - (unsigned long)cdread_audio); + (__force unsigned long)cdread_audio); }
static int compat_cdrom_generic_command(struct block_device *bdev, fmode_t mode, @@ -170,7 +170,8 @@ static int compat_cdrom_generic_command(struct block_device *bdev, fmode_t mode, put_user(compat_ptr(data), &cgc->reserved[0])) return -EFAULT;
- return __blkdev_driver_ioctl(bdev, mode, cmd, (unsigned long)cgc); + return __blkdev_driver_ioctl(bdev, mode, cmd, + (__force unsigned long)cgc); }
struct compat_blkpg_ioctl_arg { @@ -199,7 +200,7 @@ static int compat_blkpg_ioctl(struct block_device *bdev, fmode_t mode, if (err) return err;
- return blkdev_ioctl(bdev, mode, cmd, (unsigned long)a); + return blkdev_ioctl(bdev, mode, cmd, (__force unsigned long)a); }
#define BLKBSZGET_32 _IOR(0x12, 112, int) @@ -276,7 +277,7 @@ static int compat_blkdev_driver_ioctl(struct block_device *bdev, fmode_t mode, case DVD_READ_STRUCT: case DVD_WRITE_STRUCT: case DVD_AUTH: - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); /* These intepret arg as an unsigned long, not as a pointer, * so we must not do compat_ptr() conversion. */ case HDIO_SET_MULTCOUNT: @@ -355,10 +356,10 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg) */ case BLKRRPART: return blkdev_ioctl(bdev, mode, cmd, - (unsigned long)compat_ptr(arg)); + (__force unsigned long)compat_ptr(arg)); case BLKBSZSET_32: return blkdev_ioctl(bdev, mode, BLKBSZSET, - (unsigned long)compat_ptr(arg)); + (__force unsigned long)compat_ptr(arg)); case BLKPG: return compat_blkpg_ioctl(bdev, mode, cmd, compat_ptr(arg)); case BLKRAGET: diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 1984fc78c750..9d4528ec8b43 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -792,7 +792,7 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev, return put_user(val, (unsigned long __user *)arg);
case HDIO_SET_32BIT: - val = (unsigned long) arg; + val = (__force unsigned long) arg; rc = 0; spin_lock_irqsave(ap->lock, flags); if (ap->pflags & ATA_PFLAG_PIO32CHANGE) { diff --git a/drivers/block/loop.c b/drivers/block/loop.c index ea9debf59b22..910f7910ab12 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1608,7 +1608,7 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode, case LOOP_CLR_FD: case LOOP_GET_STATUS64: case LOOP_SET_STATUS64: - arg = (unsigned long) compat_ptr(arg); + arg = (__force unsigned long) compat_ptr(arg); /* fall through */ case LOOP_SET_FD: case LOOP_CHANGE_FD: diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e8f8a1999393..1f678dffe159 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -477,7 +477,8 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd, static long linehandle_ioctl_compat(struct file *filep, unsigned int cmd, unsigned long arg) { - return linehandle_ioctl(filep, cmd, (unsigned long)compat_ptr(arg)); + return linehandle_ioctl(filep, cmd, + (__force unsigned long)compat_ptr(arg)); } #endif
@@ -792,7 +793,8 @@ static long lineevent_ioctl(struct file *filep, unsigned int cmd, static long lineevent_ioctl_compat(struct file *filep, unsigned int cmd, unsigned long arg) { - return lineevent_ioctl(filep, cmd, (unsigned long)compat_ptr(arg)); + return lineevent_ioctl(filep, cmd, + (__force unsigned long)compat_ptr(arg)); } #endif
@@ -1091,7 +1093,7 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) static long gpio_ioctl_compat(struct file *filp, unsigned int cmd, unsigned long arg) { - return gpio_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); + return gpio_ioctl(filp, cmd, (__force unsigned long)compat_ptr(arg)); } #endif
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 370206f987f9..61947c834e01 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -1108,7 +1108,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, return 0;
case EVIOCRMFF: - return input_ff_erase(dev, (int)(unsigned long) p, file); + return input_ff_erase(dev, (int)(__force unsigned long)p, file);
case EVIOCGEFFECTS: i = test_bit(EV_FF, dev->evbit) ? diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index c4e7ebfe4d29..ec97b26cbd72 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -2180,7 +2180,8 @@ static long dvb_frontend_compat_ioctl(struct file *file, unsigned int cmd, return err; }
- return dvb_frontend_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); + return dvb_frontend_ioctl(file, cmd, + (__force unsigned long)compat_ptr(arg)); } #endif
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 6481212fda77..97a4e84e2070 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -505,7 +505,8 @@ static int get_v4l2_plane32(struct v4l2_plane __user *p64, break; case V4L2_MEMORY_USERPTR: if (get_user(p, &p32->m.userptr) || - put_user((unsigned long)compat_ptr(p), &p64->m.userptr)) + put_user((__force unsigned long)compat_ptr(p), + &p64->m.userptr)) return -EFAULT; break; case V4L2_MEMORY_DMABUF: @@ -657,7 +658,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64, compat_ulong_t userptr;
if (get_user(userptr, &p32->m.userptr) || - put_user((unsigned long)compat_ptr(userptr), + put_user((__force unsigned long)compat_ptr(userptr), &p64->m.userptr)) return -EFAULT; break; @@ -1340,9 +1341,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar * Otherwise, it will pass the newly allocated @new_p64 argument. */ if (compatible_arg) - err = native_ioctl(file, cmd, (unsigned long)p32); + err = native_ioctl(file, cmd, (__force unsigned long)p32); else - err = native_ioctl(file, cmd, (unsigned long)new_p64); + err = native_ioctl(file, cmd, (__force unsigned long)new_p64);
if (err == -ENOTTY) return err; diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index a0b9102c4c6e..eb2c21b55fe6 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -799,7 +799,8 @@ static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode, static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, unsigned long arg) { - return mmc_blk_ioctl(bdev, mode, cmd, (unsigned long) compat_ptr(arg)); + return mmc_blk_ioctl(bdev, mode, cmd, + (__force unsigned long) compat_ptr(arg)); } #endif
@@ -2491,7 +2492,8 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned int cmd, static long mmc_rpmb_ioctl_compat(struct file *filp, unsigned int cmd, unsigned long arg) { - return mmc_rpmb_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); + return mmc_rpmb_ioctl(filp, cmd, + (__force unsigned long)compat_ptr(arg)); } #endif
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 02389528f622..d493647821d5 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -1090,7 +1090,7 @@ static long mtdchar_compat_ioctl(struct file *file, unsigned int cmd, }
default: - ret = mtdchar_ioctl(file, cmd, (unsigned long)argp); + ret = mtdchar_ioctl(file, cmd, (__force unsigned long)argp); }
mutex_unlock(&mtd_mutex); diff --git a/drivers/net/tap.c b/drivers/net/tap.c index f0f7cd977667..eb710bc2d19d 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -1128,7 +1128,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, static long tap_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { - return tap_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); + return tap_ioctl(file, cmd, (__force unsigned long)compat_ptr(arg)); } #endif
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index ebd07ad82431..29be50de0d3d 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -3191,7 +3191,7 @@ static long tun_chr_compat_ioctl(struct file *file, case TUNSETSNDBUF: case SIOCGIFHWADDR: case SIOCSIFHWADDR: - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); break; default: arg = (compat_ulong_t)arg; diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index cda10719d1d1..d9a91676a406 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -524,8 +524,8 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
/* Convert buffer pointers */ for (n = 0; n < n_ioc; n++) { - ioc[n].rx_buf = (uintptr_t) compat_ptr(ioc[n].rx_buf); - ioc[n].tx_buf = (uintptr_t) compat_ptr(ioc[n].tx_buf); + ioc[n].rx_buf = (__force uintptr_t) compat_ptr(ioc[n].rx_buf); + ioc[n].tx_buf = (__force uintptr_t) compat_ptr(ioc[n].tx_buf); }
/* translate to spi_message, execute */ @@ -546,7 +546,7 @@ spidev_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) && _IOC_DIR(cmd) == _IOC_WRITE) return spidev_compat_ioc_message(filp, cmd, arg);
- return spidev_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); + return spidev_ioctl(filp, cmd, (__force unsigned long)compat_ptr(arg)); } #else #define spidev_compat_ioctl NULL diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index d99fec44036c..5abd148bb459 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -949,7 +949,8 @@ long n_tty_compat_ioctl_helper(struct tty_struct *tty, struct file *file, switch (cmd) { case TIOCGLCKTRMIOS: case TIOCSLCKTRMIOS: - return tty_mode_ioctl(tty, file, cmd, (unsigned long) compat_ptr(arg)); + return tty_mode_ioctl(tty, file, cmd, + (__force unsigned long) compat_ptr(arg)); default: return -ENOIOCTLCMD; } diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c index a78ad10a119b..794445d10965 100644 --- a/drivers/tty/vt/vt_ioctl.c +++ b/drivers/tty/vt/vt_ioctl.c @@ -1132,7 +1132,8 @@ compat_kdfontop_ioctl(struct compat_console_font_op __user *fontop, i = con_font_op(vc, op); if (i) return i; - ((struct compat_console_font_op *)op)->data = (unsigned long)op->data; + ((struct compat_console_font_op *)op)->data = + (__force unsigned long)op->data; if (copy_to_user(fontop, op, sizeof(struct compat_console_font_op))) return -EFAULT; return 0; @@ -1239,7 +1240,7 @@ long vt_compat_ioctl(struct tty_struct *tty, * but we have to convert it to a proper 64 bit pointer. */ default: - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); goto fallback; } out: diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index ed5ab7c8100b..e2fd6ca2d7a3 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1405,7 +1405,8 @@ find_memory_area(struct usb_dev_state *ps, const struct usbdevfs_urb *uurb) { struct usb_memory *usbm = NULL, *iter; unsigned long flags; - unsigned long uurb_start = (unsigned long)untagged_addr(uurb->buffer); + unsigned long uurb_start = + (__force unsigned long)untagged_addr(uurb->buffer);
spin_lock_irqsave(&ps->lock, flags); list_for_each_entry(iter, &ps->memory_list, memlist) { @@ -1635,7 +1636,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb } else if (uurb->buffer_length > 0) { if (as->usbm) { unsigned long uurb_start = - (unsigned long)untagged_addr(uurb->buffer); + (__force unsigned long)untagged_addr( + uurb->buffer);
as->urb->transfer_buffer = as->usbm->mem + (uurb_start - as->usbm->vm_start); @@ -1715,7 +1717,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb as->userurb = arg; if (as->usbm) { unsigned long uurb_start = - (unsigned long)untagged_addr(uurb->buffer); + (__force unsigned long)untagged_addr(uurb->buffer);
as->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; as->urb->transfer_dma = as->usbm->dma_handle + diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 64833879f75d..8f69175eba0e 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1204,7 +1204,7 @@ static long vfio_fops_unl_ioctl(struct file *filep, static long vfio_fops_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) { - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); return vfio_fops_unl_ioctl(filep, cmd, arg); } #endif /* CONFIG_COMPAT */ @@ -1576,7 +1576,7 @@ static long vfio_group_fops_unl_ioctl(struct file *filep, static long vfio_group_fops_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) { - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); return vfio_group_fops_unl_ioctl(filep, cmd, arg); } #endif /* CONFIG_COMPAT */ @@ -1707,7 +1707,7 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma) static long vfio_device_fops_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) { - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); return vfio_device_fops_unl_ioctl(filep, cmd, arg); } #endif /* CONFIG_COMPAT */ diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 20405421a5ed..a267f41378c9 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1274,7 +1274,7 @@ static int fb_getput_cmap(struct fb_info *info, unsigned int cmd, put_user(compat_ptr(data), &cmap->transp)) return -EFAULT;
- err = do_fb_ioctl(info, cmd, (unsigned long) cmap); + err = do_fb_ioctl(info, cmd, (__force unsigned long) cmap);
if (!err) { if (copy_in_user(&cmap32->start, @@ -1346,7 +1346,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, case FBIOPAN_DISPLAY: case FBIOGET_CON2FBMAP: case FBIOPUT_CON2FBMAP: - arg = (unsigned long) compat_ptr(arg); + arg = (__force unsigned long) compat_ptr(arg); /* fall through */ case FBIOBLANK: ret = do_fb_ioctl(info, cmd, arg); diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 57390c7666e5..fc5f60935f92 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -843,7 +843,7 @@ struct gntdev_copy_batch { static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user *virt, bool writeable, unsigned long *gfn) { - unsigned long addr = (unsigned long)virt; + unsigned long addr = (__force unsigned long)virt; struct page *page; unsigned long xen_pfn; int ret; @@ -953,7 +953,7 @@ static int gntdev_grant_copy_seg(struct gntdev_copy_batch *batch, op->flags |= GNTCOPY_source_gref; } else { virt = seg->source.virt + copied; - off = (unsigned long)virt & ~XEN_PAGE_MASK; + off = (__force unsigned long)virt & ~XEN_PAGE_MASK; len = min(len, (size_t)XEN_PAGE_SIZE - off);
ret = gntdev_get_page(batch, virt, false, &gfn); @@ -972,7 +972,7 @@ static int gntdev_grant_copy_seg(struct gntdev_copy_batch *batch, op->flags |= GNTCOPY_dest_gref; } else { virt = seg->dest.virt + copied; - off = (unsigned long)virt & ~XEN_PAGE_MASK; + off = (__force unsigned long)virt & ~XEN_PAGE_MASK; len = min(len, (size_t)XEN_PAGE_SIZE - off);
ret = gntdev_get_page(batch, virt, true, &gfn); diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 7e6e682104dc..d4b5dab8a80f 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -558,7 +558,7 @@ static long privcmd_ioctl_mmap_batch(
if (state.global_error) { /* Write back errors in second pass. */ - state.user_gfn = (xen_pfn_t *)m.arr; + state.user_gfn = (xen_pfn_t __user *)m.arr; state.user_err = m.err; ret = traverse_pages_block(m.num, sizeof(xen_pfn_t), &pagelist, mmap_return_errors, &state); @@ -596,7 +596,7 @@ static int lock_pages( return -ENOSPC;
pinned = get_user_pages_fast( - (unsigned long) kbufs[i].uptr, + (__force unsigned long) kbufs[i].uptr, requested, FOLL_WRITE, pages); if (pinned < 0) return pinned; diff --git a/fs/aio.c b/fs/aio.c index b9350f3360c6..bcf431f3e029 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1084,7 +1084,7 @@ static void aio_complete(struct aio_kiocb *iocb, long res, long res2) ev_page = kmap_atomic(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]); event = ev_page + pos % AIO_EVENTS_PER_PAGE;
- event->obj = (u64)(unsigned long)iocb->ki_user_iocb; + event->obj = (u64)(__force unsigned long)iocb->ki_user_iocb; event->data = iocb->ki_user_data; event->res = res; event->res2 = res2; diff --git a/fs/autofs/dev-ioctl.c b/fs/autofs/dev-ioctl.c index 86eafda4a652..f30c7dfec42b 100644 --- a/fs/autofs/dev-ioctl.c +++ b/fs/autofs/dev-ioctl.c @@ -709,7 +709,8 @@ static long autofs_dev_ioctl(struct file *file, unsigned int command, static long autofs_dev_ioctl_compat(struct file *file, unsigned int command, unsigned long u) { - return autofs_dev_ioctl(file, command, (unsigned long) compat_ptr(u)); + return autofs_dev_ioctl(file, command, + (__force unsigned long) compat_ptr(u)); } #else #define autofs_dev_ioctl_compat NULL diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 782e57b911ab..c8ebdeab6708 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -956,7 +956,7 @@ static long autofs_root_compat_ioctl(struct file *filp, ret = autofs_root_ioctl_unlocked(inode, filp, cmd, arg); else ret = autofs_root_ioctl_unlocked(inode, filp, cmd, - (unsigned long) compat_ptr(arg)); + (__force unsigned long) compat_ptr(arg));
return ret; } diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index efae2fb0930a..0292555d19a4 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -258,18 +258,18 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid)); NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid)); NEW_AUX_ENT(AT_SECURE, bprm->secureexec); - NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes); + NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(__force unsigned long)u_rand_bytes); #ifdef ELF_HWCAP2 NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2); #endif NEW_AUX_ENT(AT_EXECFN, bprm->exec); if (k_platform) { NEW_AUX_ENT(AT_PLATFORM, - (elf_addr_t)(unsigned long)u_platform); + (elf_addr_t)(__force unsigned long)u_platform); } if (k_base_platform) { NEW_AUX_ENT(AT_BASE_PLATFORM, - (elf_addr_t)(unsigned long)u_base_platform); + (elf_addr_t)(__force unsigned long)u_base_platform); } if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) { NEW_AUX_ENT(AT_EXECFD, bprm->interp_data); @@ -285,12 +285,12 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, sp = STACK_ADD(p, ei_index);
items = (argc + 1) + (envc + 1) + 1; - bprm->p = STACK_ROUND(sp, items); + bprm->p = STACK_ROUND((__force unsigned long)sp, items);
/* Point sp at the lowest address on the stack */ #ifdef CONFIG_STACK_GROWSUP sp = (elf_addr_t __user *)bprm->p - items - ei_index; - bprm->exec = (unsigned long)sp; /* XXX: PARISC HACK */ + bprm->exec = (__force unsigned long)sp; /* XXX: PARISC HACK */ #else sp = (elf_addr_t __user *)bprm->p; #endif diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 63600dc2ac4c..da884159b169 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -5971,6 +5971,6 @@ long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; }
- return btrfs_ioctl(file, cmd, (unsigned long) compat_ptr(arg)); + return btrfs_ioctl(file, cmd, (__force unsigned long) compat_ptr(arg)); } #endif diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index a9b00942e87d..675a5e862a68 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -152,7 +152,7 @@ static int do_video_get_event(struct file *file, if (kevent == NULL) return -EFAULT;
- err = do_ioctl(file, cmd, (unsigned long)kevent); + err = do_ioctl(file, cmd, (__force unsigned long)kevent); if (!err) { err = convert_in_user(&kevent->type, &up->type); err |= convert_in_user(&kevent->timestamp, &up->timestamp); @@ -193,7 +193,7 @@ static int do_video_stillpicture(struct file *file, if (err) return -EFAULT;
- err = do_ioctl(file, cmd, (unsigned long) up_native); + err = do_ioctl(file, cmd, (__force unsigned long) up_native);
return err; } @@ -264,7 +264,7 @@ static int sg_ioctl_trans(struct file *file, unsigned int cmd, if (get_user(interface_id, &sgio32->interface_id)) return -EFAULT; if (interface_id != 'S') - return do_ioctl(file, cmd, (unsigned long)sgio32); + return do_ioctl(file, cmd, (__force unsigned long)sgio32);
if (get_user(iovec_count, &sgio32->iovec_count)) return -EFAULT; @@ -324,7 +324,7 @@ static int sg_ioctl_trans(struct file *file, unsigned int cmd, if (put_user(compat_ptr(data), &sgio->usr_ptr)) return -EFAULT;
- err = do_ioctl(file, cmd, (unsigned long) sgio); + err = do_ioctl(file, cmd, (__force unsigned long) sgio);
if (err >= 0) { void __user *datap; @@ -332,7 +332,7 @@ static int sg_ioctl_trans(struct file *file, unsigned int cmd, if (copy_in_user(&sgio32->pack_id, &sgio->pack_id, sizeof(int)) || get_user(datap, &sgio->usr_ptr) || - put_user((u32)(unsigned long)datap, + put_user((u32)(__force unsigned long)datap, &sgio32->usr_ptr) || copy_in_user(&sgio32->status, &sgio->status, (4 * sizeof(unsigned char)) + @@ -361,7 +361,7 @@ static int sg_grt_trans(struct file *file, int err, i; sg_req_info_t __user *r; r = compat_alloc_user_space(sizeof(sg_req_info_t)*SG_MAX_QUEUE); - err = do_ioctl(file, cmd, (unsigned long)r); + err = do_ioctl(file, cmd, (__force unsigned long)r); if (err < 0) return err; for (i = 0; i < SG_MAX_QUEUE; i++) { @@ -371,7 +371,7 @@ static int sg_grt_trans(struct file *file, if (copy_in_user(o + i, r + i, offsetof(sg_req_info_t, usr_ptr)) || get_user(ptr, &r[i].usr_ptr) || get_user(d, &r[i].duration) || - put_user((u32)(unsigned long)(ptr), &o[i].usr_ptr) || + put_user((u32)(__force unsigned long)(ptr), &o[i].usr_ptr) || put_user(d, &o[i].duration)) return -EFAULT; } @@ -410,7 +410,7 @@ static int ppp_sock_fprog_ioctl_trans(struct file *file, else cmd = PPPIOCSACTIVE;
- return do_ioctl(file, cmd, (unsigned long) u_fprog64); + return do_ioctl(file, cmd, (__force unsigned long) u_fprog64); }
struct ppp_option_data32 { @@ -435,7 +435,7 @@ static int ppp_gidle(struct file *file, unsigned int cmd,
idle = compat_alloc_user_space(sizeof(*idle));
- err = do_ioctl(file, PPPIOCGIDLE, (unsigned long) idle); + err = do_ioctl(file, PPPIOCGIDLE, (__force unsigned long) idle);
if (!err) { if (get_user(xmit, &idle->xmit_idle) || @@ -467,7 +467,7 @@ static int ppp_scompress(struct file *file, unsigned int cmd, sizeof(__u32) + sizeof(int))) return -EFAULT;
- return do_ioctl(file, PPPIOCSCOMPRESS, (unsigned long) odata); + return do_ioctl(file, PPPIOCSCOMPRESS, (__force unsigned long) odata); }
#ifdef CONFIG_BLOCK @@ -607,7 +607,7 @@ static int serial_struct_ioctl(struct file *file, put_user(0UL, &ss->iomap_base)) return -EFAULT; } - err = do_ioctl(file, cmd, (unsigned long)ss); + err = do_ioctl(file, cmd, (__force unsigned long)ss); if (cmd == TIOCGSERIAL && err >= 0) { if (copy_in_user(ss32, ss, offsetof(SS32, iomem_base)) || get_user(iomem_base, &ss->iomem_base)) @@ -641,14 +641,16 @@ static int rtc_ioctl(struct file *file, case RTC_EPOCH_READ32: ret = do_ioctl(file, (cmd == RTC_IRQP_READ32) ? RTC_IRQP_READ : RTC_EPOCH_READ, - (unsigned long)valp); + (__force unsigned long)valp); if (ret) return ret; return convert_in_user(valp, (unsigned int __user *)argp); case RTC_IRQP_SET32: - return do_ioctl(file, RTC_IRQP_SET, (unsigned long)argp); + return do_ioctl(file, RTC_IRQP_SET, + (__force unsigned long)argp); case RTC_EPOCH_SET32: - return do_ioctl(file, RTC_EPOCH_SET, (unsigned long)argp); + return do_ioctl(file, RTC_EPOCH_SET, + (__force unsigned long)argp); }
return -ENOIOCTLCMD; @@ -1436,7 +1438,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, goto out_fput;
found_handler: - arg = (unsigned long)compat_ptr(arg); + arg = (__force unsigned long)compat_ptr(arg); do_ioctl: error = do_vfs_ioctl(f.file, fd, cmd, arg); out_fput: diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c index 0367c0039e68..5cf6e2666107 100644 --- a/fs/ext2/ioctl.c +++ b/fs/ext2/ioctl.c @@ -183,6 +183,6 @@ long ext2_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) default: return -ENOIOCTLCMD; } - return ext2_ioctl(file, cmd, (unsigned long) compat_ptr(arg)); + return ext2_ioctl(file, cmd, (__force unsigned long) compat_ptr(arg)); } #endif diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index a7074115d6f6..02c9ffbbb209 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -1107,6 +1107,6 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) default: return -ENOIOCTLCMD; } - return ext4_ioctl(file, cmd, (unsigned long) compat_ptr(arg)); + return ext4_ioctl(file, cmd, (__force unsigned long) compat_ptr(arg)); } #endif diff --git a/fs/fat/file.c b/fs/fat/file.c index 4f3d72fb1e60..88f267d5042f 100644 --- a/fs/fat/file.c +++ b/fs/fat/file.c @@ -176,7 +176,8 @@ static long fat_generic_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{ - return fat_generic_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); + return fat_generic_ioctl(filp, cmd, + (__force unsigned long)compat_ptr(arg)); } #endif
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 32d0b883e74f..4c0ccfeb2e24 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1255,7 +1255,7 @@ static inline void fuse_page_descs_length_init(struct fuse_req *req,
static inline unsigned long fuse_get_user_addr(const struct iov_iter *ii) { - return (unsigned long)ii->iov->iov_base + ii->iov_offset; + return (__force unsigned long)ii->iov->iov_base + ii->iov_offset; }
static inline size_t fuse_get_frag_size(const struct iov_iter *ii, diff --git a/fs/namespace.c b/fs/namespace.c index 51f763fb9430..8307bd0399f3 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2672,7 +2672,7 @@ void *copy_mount_options(const void __user * data) * the remainder of the page. */ /* copy_from_user cannot cross TASK_SIZE ! */ - size = TASK_SIZE - (unsigned long)untagged_addr(data); + size = TASK_SIZE - (__force unsigned long)untagged_addr(data); if (size > PAGE_SIZE) size = PAGE_SIZE;
diff --git a/fs/readdir.c b/fs/readdir.c index d97f548e6323..d5de36c3cc66 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -366,8 +366,8 @@ static int compat_fillonedir(struct dir_context *ctx, const char *name, buf->result++; dirent = buf->dirent; if (!access_ok(VERIFY_WRITE, dirent, - (unsigned long)(dirent->d_name + namlen + 1) - - (unsigned long)dirent)) + (__force unsigned long)(dirent->d_name + namlen + 1) - + (__force unsigned long)dirent)) goto efault; if ( __put_user(d_ino, &dirent->d_ino) || __put_user(offset, &dirent->d_offset) || diff --git a/fs/signalfd.c b/fs/signalfd.c index 4fcd1498acf5..23bc1d4d870a 100644 --- a/fs/signalfd.c +++ b/fs/signalfd.c @@ -105,7 +105,7 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, case SIL_TIMER: new.ssi_tid = kinfo->si_tid; new.ssi_overrun = kinfo->si_overrun; - new.ssi_ptr = (long) kinfo->si_ptr; + new.ssi_ptr = (__force long) kinfo->si_ptr; new.ssi_int = kinfo->si_int; break; case SIL_POLL: @@ -122,13 +122,13 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, * it as SIL_FAULT. */ case SIL_FAULT: - new.ssi_addr = (long) kinfo->si_addr; + new.ssi_addr = (__force long) kinfo->si_addr; #ifdef __ARCH_SI_TRAPNO new.ssi_trapno = kinfo->si_trapno; #endif break; case SIL_FAULT_MCEERR: - new.ssi_addr = (long) kinfo->si_addr; + new.ssi_addr = (__force long) kinfo->si_addr; #ifdef __ARCH_SI_TRAPNO new.ssi_trapno = kinfo->si_trapno; #endif @@ -147,11 +147,11 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, */ new.ssi_pid = kinfo->si_pid; new.ssi_uid = kinfo->si_uid; - new.ssi_ptr = (long) kinfo->si_ptr; + new.ssi_ptr = (__force long) kinfo->si_ptr; new.ssi_int = kinfo->si_int; break; case SIL_SYS: - new.ssi_call_addr = (long) kinfo->si_call_addr; + new.ssi_call_addr = (__force long) kinfo->si_call_addr; new.ssi_syscall = kinfo->si_syscall; new.ssi_arch = kinfo->si_arch; break; diff --git a/include/linux/mm.h b/include/linux/mm.h index a61ebe8ad4ca..0cfd80983fea 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1297,7 +1297,7 @@ static inline void clear_page_pfmemalloc(struct page *page) */ extern void pagefault_out_of_memory(void);
-#define offset_in_page(p) ((unsigned long)(p) & ~PAGE_MASK) +#define offset_in_page(p) ((__force unsigned long)(p) & ~PAGE_MASK)
/* * Flags passed to show_mem() and show_free_areas() to suppress output in diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index b1bd2186e6d2..26d08d4ed59b 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -579,8 +579,8 @@ static inline int fault_in_pages_writeable(char __user *uaddr, int size) } while (uaddr <= end);
/* Check whether the range spilled into the next page. */ - if (((unsigned long)uaddr & PAGE_MASK) == - ((unsigned long)end & PAGE_MASK)) + if (((__force unsigned long)uaddr & PAGE_MASK) == + ((__force unsigned long)end & PAGE_MASK)) return __put_user(0, end);
return 0; @@ -604,8 +604,8 @@ static inline int fault_in_pages_readable(const char __user *uaddr, int size) } while (uaddr <= end);
/* Check whether the range spilled into the next page. */ - if (((unsigned long)uaddr & PAGE_MASK) == - ((unsigned long)end & PAGE_MASK)) { + if (((__force unsigned long)uaddr & PAGE_MASK) == + ((__force unsigned long)end & PAGE_MASK)) { return __get_user(c, end); }
diff --git a/include/linux/socket.h b/include/linux/socket.h index 7ed4713d5337..529f526bca1c 100644 --- a/include/linux/socket.h +++ b/include/linux/socket.h @@ -93,7 +93,7 @@ struct cmsghdr {
#define CMSG_ALIGN(len) ( ((len)+sizeof(long)-1) & ~(sizeof(long)-1) )
-#define CMSG_DATA(cmsg) ((void *)((char *)(cmsg) + sizeof(struct cmsghdr))) +#define CMSG_DATA(cmsg) ((void *)((char __force *)(cmsg) + sizeof(struct cmsghdr))) #define CMSG_SPACE(len) (sizeof(struct cmsghdr) + CMSG_ALIGN(len)) #define CMSG_LEN(len) (sizeof(struct cmsghdr) + (len))
diff --git a/ipc/shm.c b/ipc/shm.c index b0eb3757ab89..310096ffe8c4 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1392,7 +1392,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, unsigned long shmlba) { struct shmid_kernel *shp; - unsigned long addr = (unsigned long)shmaddr; + unsigned long addr = (__force unsigned long)shmaddr; unsigned long size; struct file *file, *base; int err; @@ -1600,7 +1600,7 @@ long ksys_shmdt(char __user *shmaddr) { struct mm_struct *mm = current->mm; struct vm_area_struct *vma; - unsigned long addr = (unsigned long)shmaddr; + unsigned long addr = (__force unsigned long)shmaddr; int retval = -EINVAL; #ifdef CONFIG_MMU loff_t size = 0; diff --git a/kernel/futex.c b/kernel/futex.c index 11fc3bb456d6..8bb0858c795a 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -499,7 +499,7 @@ static void drop_futex_key_refs(union futex_key *key) static int get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) { - unsigned long address = (unsigned long)uaddr; + unsigned long address = (__force unsigned long)uaddr; struct mm_struct *mm = current->mm; struct page *page, *tail; struct address_space *mapping; @@ -727,7 +727,7 @@ static int fault_in_user_writeable(u32 __user *uaddr) int ret;
down_read(&mm->mmap_sem); - ret = fixup_user_fault(current, mm, (unsigned long)uaddr, + ret = fixup_user_fault(current, mm, (__force unsigned long)uaddr, FAULT_FLAG_WRITE, NULL); up_read(&mm->mmap_sem);
@@ -3584,7 +3584,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, */ if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE || cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP) - val2 = (u32) (unsigned long) utime; + val2 = (u32) (__force unsigned long) utime;
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3); } diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c index 83f830acbb5f..b6052ae7b349 100644 --- a/kernel/futex_compat.c +++ b/kernel/futex_compat.c @@ -196,7 +196,7 @@ COMPAT_SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, } if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE || cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP) - val2 = (int) (unsigned long) utime; + val2 = (int) (__force unsigned long) utime;
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3); } diff --git a/kernel/power/user.c b/kernel/power/user.c index 2d8b60a3c86b..10a578efc892 100644 --- a/kernel/power/user.c +++ b/kernel/power/user.c @@ -431,7 +431,7 @@ snapshot_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case SNAPSHOT_CREATE_IMAGE: return snapshot_ioctl(file, cmd, - (unsigned long) compat_ptr(arg)); + (__force unsigned long) compat_ptr(arg));
case SNAPSHOT_SET_SWAP_AREA: { struct compat_resume_swap_area __user *u_swap_area = diff --git a/kernel/signal.c b/kernel/signal.c index 5843c541fda9..2bc6d7fdeaec 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3494,7 +3494,7 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp) return -ENOMEM; }
- t->sas_ss_sp = (unsigned long) ss_sp; + t->sas_ss_sp = (__force unsigned long) ss_sp; t->sas_ss_size = ss_size; t->sas_ss_flags = ss_flags; } diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 8be175df3075..6b1f373d241d 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1112,9 +1112,9 @@ unsigned long iov_iter_alignment(const struct iov_iter *i) return size; } iterate_all_kinds(i, size, v, - (res |= (unsigned long)v.iov_base | v.iov_len, 0), + (res |= (__force unsigned long)v.iov_base | v.iov_len, 0), res |= v.bv_offset | v.bv_len, - res |= (unsigned long)v.iov_base | v.iov_len + res |= (__force unsigned long)v.iov_base | v.iov_len ) return res; } @@ -1131,11 +1131,11 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i) }
iterate_all_kinds(i, size, v, - (res |= (!res ? 0 : (unsigned long)v.iov_base) | + (res |= (!res ? 0 : (__force unsigned long)v.iov_base) | (size != v.iov_len ? size : 0), 0), (res |= (!res ? 0 : (unsigned long)v.bv_offset) | (size != v.bv_len ? size : 0)), - (res |= (!res ? 0 : (unsigned long)v.iov_base) | + (res |= (!res ? 0 : (__force unsigned long)v.iov_base) | (size != v.iov_len ? size : 0)) ); return res; @@ -1196,7 +1196,7 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, if (unlikely(i->type & ITER_PIPE)) return pipe_get_pages(i, pages, maxsize, maxpages, start); iterate_all_kinds(i, maxsize, v, ({ - unsigned long addr = (unsigned long)v.iov_base; + unsigned long addr = (__force unsigned long)v.iov_base; size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1)); int n; int res; @@ -1273,7 +1273,7 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, if (unlikely(i->type & ITER_PIPE)) return pipe_get_pages_alloc(i, pages, maxsize, start); iterate_all_kinds(i, maxsize, v, ({ - unsigned long addr = (unsigned long)v.iov_base; + unsigned long addr = (__force unsigned long)v.iov_base; size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1)); int n; int res; @@ -1457,7 +1457,7 @@ int iov_iter_npages(const struct iov_iter *i, int maxpages) if (npages >= maxpages) return maxpages; } else iterate_all_kinds(i, size, v, ({ - unsigned long p = (unsigned long)v.iov_base; + unsigned long p = (__force unsigned long)v.iov_base; npages += DIV_ROUND_UP(p + v.iov_len, PAGE_SIZE) - p / PAGE_SIZE; if (npages >= maxpages) @@ -1467,7 +1467,7 @@ int iov_iter_npages(const struct iov_iter *i, int maxpages) if (npages >= maxpages) return maxpages; }),({ - unsigned long p = (unsigned long)v.iov_base; + unsigned long p = (__force unsigned long)v.iov_base; npages += DIV_ROUND_UP(p + v.iov_len, PAGE_SIZE) - p / PAGE_SIZE; if (npages >= maxpages) diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index 97467cd2bc59..2dc90838a594 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -109,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count) src = untagged_addr(src);
max_addr = user_addr_max(); - src_addr = (unsigned long)src; + src_addr = (__force unsigned long)src; if (likely(src_addr < max_addr)) { unsigned long max = max_addr - src_addr; long retval; diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c index 8b5f56466e00..10cc31f41064 100644 --- a/lib/strnlen_user.c +++ b/lib/strnlen_user.c @@ -42,7 +42,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count, * Do everything aligned. But that means that we * need to also expand the maximum.. */ - align = (sizeof(long) - 1) & (unsigned long)src; + align = (sizeof(long) - 1) & (__force unsigned long)src; src -= align; max += align;
@@ -111,7 +111,7 @@ long strnlen_user(const char __user *str, long count) str = untagged_addr(str);
max_addr = user_addr_max(); - src_addr = (unsigned long)str; + src_addr = (__force unsigned long)str; if (likely(src_addr < max_addr)) { unsigned long max = max_addr - src_addr; long retval; diff --git a/lib/test_kasan.c b/lib/test_kasan.c index ec657105edbf..e6a6ad7cc054 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -476,7 +476,7 @@ static noinline void __init copy_user_test(void) pr_info("out-of-bounds in strncpy_from_user()\n"); unused = strncpy_from_user(kmem, usermem, size + 1);
- vm_munmap((unsigned long)usermem, PAGE_SIZE); + vm_munmap((__force unsigned long)usermem, PAGE_SIZE); kfree(kmem); }
diff --git a/mm/memory.c b/mm/memory.c index c467102a5cbc..eb7606ab3620 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4728,7 +4728,7 @@ long copy_huge_page_from_user(struct page *dst_page, unsigned int pages_per_huge_page, bool allow_pagefault) { - void *src = (void *)usr_src; + void *src = (__force void *)usr_src; void *page_kaddr; unsigned long i, rc = 0; unsigned long ret_val = pages_per_huge_page * PAGE_SIZE; diff --git a/mm/migrate.c b/mm/migrate.c index d6a2e89b086a..9786b5f827cf 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1582,7 +1582,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; if (get_user(node, nodes + i)) goto out_flush; - addr = (unsigned long)p; + addr = (__force unsigned long)p;
err = -ENODEV; if (node < 0 || node >= MAX_NUMNODES) @@ -1656,7 +1656,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages, down_read(&mm->mmap_sem);
for (i = 0; i < nr_pages; i++) { - unsigned long addr = (unsigned long)(*pages); + unsigned long addr = (__force unsigned long)(*pages); struct vm_area_struct *vma; struct page *page; int err = -EFAULT; diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c index a447092d4635..4a7a55f4614d 100644 --- a/mm/process_vm_access.c +++ b/mm/process_vm_access.c @@ -175,10 +175,10 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter, for (i = 0; i < riovcnt; i++) { iov_len = rvec[i].iov_len; if (iov_len > 0) { - nr_pages_iov = ((unsigned long)rvec[i].iov_base - + iov_len) - / PAGE_SIZE - (unsigned long)rvec[i].iov_base - / PAGE_SIZE + 1; + nr_pages_iov = ((__force unsigned long)rvec[i].iov_base + + iov_len) / PAGE_SIZE - + (__force unsigned long)rvec[i].iov_base + / PAGE_SIZE + 1; nr_pages = max(nr_pages, nr_pages_iov); } } @@ -218,8 +218,9 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
for (i = 0; i < riovcnt && iov_iter_count(iter) && !rc; i++) rc = process_vm_rw_single_vec( - (unsigned long)rvec[i].iov_base, rvec[i].iov_len, - iter, process_pages, mm, task, vm_write); + (__force unsigned long)rvec[i].iov_base, + rvec[i].iov_len, iter, process_pages, mm, task, + vm_write);
/* copied = space before - space after */ total_len -= iov_iter_count(iter); diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c index 1eaac01f85de..a07dc6ad085f 100644 --- a/net/bluetooth/hidp/sock.c +++ b/net/bluetooth/hidp/sock.c @@ -185,7 +185,7 @@ static int hidp_sock_compat_ioctl(struct socket *sock, unsigned int cmd, unsigne copy_to_user(&uca->name[0], &ca.name[0], 128)) return -EFAULT;
- arg = (unsigned long) uca; + arg = (__force unsigned long) uca;
/* Fall through. We don't actually write back any _changes_ to the structure anyway, so there's no need to copy back diff --git a/net/compat.c b/net/compat.c index 3b2105f6549d..786c71c44f99 100644 --- a/net/compat.c +++ b/net/compat.c @@ -103,7 +103,7 @@ int get_compat_msghdr(struct msghdr *kmsg, ((ucmlen) >= sizeof(struct compat_cmsghdr) && \ (ucmlen) <= (unsigned long) \ ((mhdr)->msg_controllen - \ - ((char *)(ucmsg) - (char *)(mhdr)->msg_control))) + ((char __force *)(ucmsg) - (char __force *)(mhdr)->msg_control)))
static inline struct compat_cmsghdr __user *cmsg_compat_nxthdr(struct msghdr *msg, struct compat_cmsghdr __user *cmsg, int cmsg_len) @@ -582,7 +582,7 @@ int compat_mc_setsockopt(struct sock *sock, int level, int optname, case MCAST_JOIN_GROUP: case MCAST_LEAVE_GROUP: { - struct compat_group_req __user *gr32 = (void *)optval; + struct compat_group_req __user *gr32 = (__force void *)optval; struct group_req __user *kgr = compat_alloc_user_space(sizeof(struct group_req)); u32 interface; @@ -603,7 +603,8 @@ int compat_mc_setsockopt(struct sock *sock, int level, int optname, case MCAST_BLOCK_SOURCE: case MCAST_UNBLOCK_SOURCE: { - struct compat_group_source_req __user *gsr32 = (void *)optval; + struct compat_group_source_req __user *gsr32 = + (__force void *)optval; struct group_source_req __user *kgsr = compat_alloc_user_space( sizeof(struct group_source_req)); u32 interface; @@ -624,7 +625,8 @@ int compat_mc_setsockopt(struct sock *sock, int level, int optname, } case MCAST_MSFILTER: { - struct compat_group_filter __user *gf32 = (void *)optval; + struct compat_group_filter __user *gf32 = + (__force void *)optval; struct group_filter __user *kgf; u32 interface, fmode, numsrc;
@@ -662,7 +664,7 @@ int compat_mc_getsockopt(struct sock *sock, int level, int optname, char __user *optval, int __user *optlen, int (*getsockopt)(struct sock *, int, int, char __user *, int __user *)) { - struct compat_group_filter __user *gf32 = (void *)optval; + struct compat_group_filter __user *gf32 = (__force void *)optval; struct group_filter __user *kgf; int __user *koptlen; u32 interface, fmode, numsrc; diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c index 507fd5210c1c..eae6af108bf2 100644 --- a/sound/core/control_compat.c +++ b/sound/core/control_compat.c @@ -418,7 +418,8 @@ static int snd_ctl_elem_add_compat(struct snd_ctl_file *file, sizeof(data->value.enumerated))) goto error; data->value.enumerated.names_ptr = - (uintptr_t)compat_ptr(data->value.enumerated.names_ptr); + (__force uintptr_t)compat_ptr( + data->value.enumerated.names_ptr); break; default: break; @@ -465,7 +466,7 @@ static inline long snd_ctl_ioctl_compat(struct file *file, unsigned int cmd, uns case SNDRV_CTL_IOCTL_TLV_READ: case SNDRV_CTL_IOCTL_TLV_WRITE: case SNDRV_CTL_IOCTL_TLV_COMMAND: - return snd_ctl_ioctl(file, cmd, (unsigned long)argp); + return snd_ctl_ioctl(file, cmd, (__force unsigned long)argp); case SNDRV_CTL_IOCTL_ELEM_LIST32: return snd_ctl_elem_list_compat(ctl->card, argp); case SNDRV_CTL_IOCTL_ELEM_INFO32: diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 66c90f486af9..6bf7fc29d6a0 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2888,7 +2888,8 @@ static int snd_pcm_common_ioctl(struct file *file, case SNDRV_PCM_IOCTL_START: return snd_pcm_start_lock_irq(substream); case SNDRV_PCM_IOCTL_LINK: - return snd_pcm_link(substream, (int)(unsigned long) arg); + return snd_pcm_link(substream, + (int)(__force unsigned long) arg); case SNDRV_PCM_IOCTL_UNLINK: return snd_pcm_unlink(substream); case SNDRV_PCM_IOCTL_RESUME: @@ -2925,7 +2926,7 @@ static int snd_pcm_common_ioctl(struct file *file, case SNDRV_PCM_IOCTL_PAUSE: return snd_pcm_action_lock_irq(&snd_pcm_action_pause, substream, - (int)(unsigned long)arg); + (int)(__force unsigned long)arg); case SNDRV_PCM_IOCTL_WRITEI_FRAMES: case SNDRV_PCM_IOCTL_READI_FRAMES: return snd_pcm_xferi_frames_ioctl(substream, arg); diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c index e00f7e399e46..b7bf8a99f883 100644 --- a/sound/core/timer_compat.c +++ b/sound/core/timer_compat.c @@ -154,7 +154,8 @@ static long __snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, case SNDRV_TIMER_IOCTL_PAUSE: case SNDRV_TIMER_IOCTL_PAUSE_OLD: case SNDRV_TIMER_IOCTL_NEXT_DEVICE: - return __snd_timer_user_ioctl(file, cmd, (unsigned long)argp); + return __snd_timer_user_ioctl(file, cmd, + (__force unsigned long)argp); case SNDRV_TIMER_IOCTL_GPARAMS32: return snd_timer_user_gparams_compat(file, argp); case SNDRV_TIMER_IOCTL_INFO32:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
-- Luc Van Oostenryck
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
It would be even more useful if that series would either deal with the noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doing the right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be. They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
On 03/09/18 13:34, Andrey Konovalov wrote:
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I think as well that we should make explicit the information that __force is hiding. A possible solution could be defining some new address spaces and use them where it is relevant in the kernel. Something like:
# define __compat_ptr __attribute__((noderef, address_space(5))) # define __tagged_ptr __attribute__((noderef, address_space(6)))
In this way sparse can still identify the casting and trigger a warning.
We could at that point modify sparse to ignore these conversions when a specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr) to exclude from the generated warnings the ones we have already dealt with.
What do you think about this approach?
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
On 03/09/18 13:34, Andrey Konovalov wrote:
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I think as well that we should make explicit the information that __force is hiding. A possible solution could be defining some new address spaces and use them where it is relevant in the kernel. Something like:
# define __compat_ptr __attribute__((noderef, address_space(5))) # define __tagged_ptr __attribute__((noderef, address_space(6)))
In this way sparse can still identify the casting and trigger a warning.
We could at that point modify sparse to ignore these conversions when a specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr) to exclude from the generated warnings the ones we have already dealt with.
What do you think about this approach?
I'll be happy to add such warnings to sparse if it is useful to detect (and correct!) problems. I'm also thinking to other possiblities, like having some weaker form of __force (maybe simply __force_as (which will 'only' force the address space) or even __force_as(TO, FROM) (with TO and FROM being a mask of the address space allowed).
However, for the specific situation here, I'm not sure that using address spaces is the right choice because I suspect that the concept of tagged pointer is orthogonal to the one of (the usual) address space (it won't be possible for a pointer to be __tagged_ptr *and* __user).
OTOH, when I see already the tons of warnings for concepts established since many years (I'm thinking especially at __bitwise, see [1]) I'm a bit affraid of adding new, more specialized ones that people will understand even less how/when they need to use them.
-- Luc
[1] Here are the warnings reported on v18-rc1 x86-64 with defconfig: 469 symbol was not declared. Should it be static? 241 incorrect type in argument (different address spaces) 186 context imbalance - unexpected unlock 147 restricted type degrades to integer 122 incompatible types in comparison expression (different address spaces) 117 context imbalance - different lock contexts for basic block 102 incorrect type in assignment (different address spaces) 101 incorrect type in initializer (different address spaces) 82 dereference of noderef expression 79 cast to restricted type 74 incorrect type in argument (different base types) 72 bad integer constant expression 68 context imbalance - wrong count at exit 65 incorrect type in assignment (different base types) 44 cast removes address space of expression 38 Using plain integer as NULL pointer 20 Variable length array is used. 14 symbol redeclared with different type - different modifiers 14 cast from restricted type 13 function with external linkage has definition 12 subtraction of functions? Share your drugs 11 directive in argument list 8 incorrect type in return expression (different address spaces) 6 cast truncates bits from constant value 5 invalid assignement 5 incorrect type in return expression (different base types) 5 incorrect type in initializer (different base types) 4 "Sparse checking disabled for this file" 3 memset with byte count of ... 3 incorrect type in initializer (different modifiers) 2 Initializer entry defined twice 2 incorrect type in assignment (different modifiers) 2 incorrect type in argument (different modifiers) 2 arithmetics on pointers to functions 1 trying to concatenate long character string (8191 bytes max) 1 too long token expansion 1 symbol redeclared with different type - incompatible argument (different address spaces) 1 memcpy with byte count of ... 1 marked inline, but without a definition 1 invalid initializer 1 incorrect type in argument (incompatible argument (different signedness)) 1 incompatible types in comparison expression (different base types) 1 dubious: !x | y 1 constant is so big it is ...
On 03/09/18 16:10, Luc Van Oostenryck wrote:
On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
On 03/09/18 13:34, Andrey Konovalov wrote:
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I think as well that we should make explicit the information that __force is hiding. A possible solution could be defining some new address spaces and use them where it is relevant in the kernel. Something like:
# define __compat_ptr __attribute__((noderef, address_space(5))) # define __tagged_ptr __attribute__((noderef, address_space(6)))
In this way sparse can still identify the casting and trigger a warning.
We could at that point modify sparse to ignore these conversions when a specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr) to exclude from the generated warnings the ones we have already dealt with.
What do you think about this approach?
I'll be happy to add such warnings to sparse if it is useful to detect (and correct!) problems. I'm also thinking to other possiblities, like having some weaker form of __force (maybe simply __force_as (which will 'only' force the address space) or even __force_as(TO, FROM) (with TO and FROM being a mask of the address space allowed).I believe we need something here to address this type of problems and I like
your proposal of adding a weaker force in the form of __force_as(TO, FROM) because I think it provides the right level information.
However, for the specific situation here, I'm not sure that using address spaces is the right choice because I suspect that the concept of tagged pointer is orthogonal to the one of (the usual) address space (it won't be possible for a pointer to be __tagged_ptr *and* __user).
I was thinking to address spaces because the information seems easily accessible in sparse [1], but I am certainly open to any solution that can be semantically more correct.
OTOH, when I see already the tons of warnings for concepts established since many years (I'm thinking especially at __bitwise, see [1]) I'm a bit affraid of adding new, more specialized ones that people will understand even less how/when they need to use them.
Thanks for providing this statistic, it is very interesting. I understand your concern, but I think that in this case we need a more specialized option not only to find potential problems but even to provide the right amount of information to who reads the code.
A solution could be to let __force_as(TO, FROM) behave like __force and silence the warning by default, but have an option in sparse to re-enable it (i.e. -Wshow-force-as).
[1] --- commit ee7985f0c2b29c96aefe78df4139209eb4e719d8 Author: Vincenzo Frascino vincenzo.frascino@arm.com Date: Wed Aug 15 10:55:44 2018 +0100
print address space number for explicit cast to ulong
This patch build on top of commit b34880d ("stricter warning for explicit cast to ulong") and prints the address space number when a "warning: cast removes address space of expression" is triggered.
This makes easier to discriminate in between different address spaces.
A validation example is provided as well as part of this patch.
Signed-off-by: Vincenzo Frascino vincenzo.frascino@arm.com
diff --git a/evaluate.c b/evaluate.c index 6d5d479..2fc0ebc 100644 --- a/evaluate.c +++ b/evaluate.c @@ -3017,8 +3017,12 @@ static struct symbol *evaluate_cast(struct expression *expr) sas = stype->ctype.as; }
- if (!tas && sas > 0) - warning(expr->pos, "cast removes address space of expression"); + if (!tas && sas > 0) { + if (Wcast_from_as) + warning(expr->pos, "cast removes address space of expression (asn:%d)", sas); + else + warning(expr->pos, "cast removes address space of expression"); + } if (tas > 0 && sas > 0 && tas != sas) warning(expr->pos, "cast between address spaces (asn:%d->asn:%d)", sas, tas); if (tas > 0 && !sas && diff --git a/sparse.1 b/sparse.1 index 3e13523..699d09f 100644 --- a/sparse.1 +++ b/sparse.1 @@ -84,6 +84,9 @@ This is similar to \fB-Waddress-space\fR but will also warn on casts to \fBunsigned long\fR.
Sparse does not issues these warnings by default. + +When the option is activated the address space number is printed +as part of the warning message. . .TP .B -Wcast-to-as diff --git a/validation/Waddress-space-all-attr.c b/validation/Waddress-space-all-attr.c new file mode 100644 index 0000000..455ba68 --- /dev/null +++ b/validation/Waddress-space-all-attr.c @@ -0,0 +1,84 @@ +/* Resembles include/linux/compiler_types.h */ +#define __kernel __attribute__((address_space(0))) +#define __user __attribute__((address_space(1))) +#define __iomem __attribute__((address_space(2))) +#define __percpu __attribute__((address_space(3))) +#define __rcu __attribute__((address_space(4))) + + +typedef unsigned long ulong; +typedef long long llong; +typedef struct s obj_t; + +static void expl(obj_t __kernel *k, obj_t __iomem *o, + obj_t __user *p, obj_t __percpu *pc, + obj_t __rcu *r) +{ + (int)(k); + (ulong)(k); + (llong)(k); + (void *)(k); + (obj_t*)(k); + (obj_t __kernel*)(k); + + (int)(o); + (ulong)(o); + (llong)(o); + (void *)(o); + (obj_t*)(o); + (obj_t __iomem*)(o); + + (int)(p); + (ulong)(p); + (llong)(p); + (void *)(p); + (obj_t*)(p); + (obj_t __user*)(p); + + (int)(pc); + (ulong)(pc); + (llong)(pc); + (void *)(pc); + (obj_t*)(pc); + (obj_t __percpu*)(pc); + + (int)(r); + (ulong)(r); + (llong)(r); + (void *)(r); + (obj_t*)(r); + (obj_t __rcu*)(r); +} + +/* + * check-name: Waddress-space-all-attr + * check-command: sparse -Wcast-from-as -Wcast-to-as $file + * + * check-error-start +Waddress-space-all-attr.c:24:10: warning: cast removes address space of expression (asn:2) +Waddress-space-all-attr.c:25:10: warning: cast removes address space of expression (asn:2) +Waddress-space-all-attr.c:26:10: warning: cast removes address space of expression (asn:2) +Waddress-space-all-attr.c:27:10: warning: cast removes address space of expression (asn:2) +Waddress-space-all-attr.c:28:10: warning: cast removes address space of expression (asn:2) +Waddress-space-all-attr.c:31:10: warning: cast removes address space of expression (asn:1) +Waddress-space-all-attr.c:32:10: warning: cast removes address space of expression (asn:1) +Waddress-space-all-attr.c:33:10: warning: cast removes address space of expression (asn:1) +Waddress-space-all-attr.c:34:10: warning: cast removes address space of expression (asn:1) +Waddress-space-all-attr.c:35:10: warning: cast removes address space of expression (asn:1) +Waddress-space-all-attr.c:38:10: warning: cast removes address space of expression (asn:3) +Waddress-space-all-attr.c:39:10: warning: cast removes address space of expression (asn:3) +Waddress-space-all-attr.c:40:10: warning: cast removes address space of expression (asn:3) +Waddress-space-all-attr.c:41:10: warning: cast removes address space of expression (asn:3) +Waddress-space-all-attr.c:42:10: warning: cast removes address space of expression (asn:3) +Waddress-space-all-attr.c:45:10: warning: cast removes address space of expression (asn:4) +Waddress-space-all-attr.c:46:10: warning: cast removes address space of expression (asn:4) +Waddress-space-all-attr.c:47:10: warning: cast removes address space of expression (asn:4) +Waddress-space-all-attr.c:48:10: warning: cast removes address space of expression (asn:4) +Waddress-space-all-attr.c:49:10: warning: cast removes address space of expression (asn:4) +Waddress-space-all-attr.c:17:10: warning: non size-preserving pointer to integer cast +Waddress-space-all-attr.c:24:10: warning: non size-preserving pointer to integer cast +Waddress-space-all-attr.c:31:10: warning: non size-preserving pointer to integer cast +Waddress-space-all-attr.c:38:10: warning: non size-preserving pointer to integer cast +Waddress-space-all-attr.c:45:10: warning: non size-preserving pointer to integer cast + * check-error-end + */
On Tue, Sep 04, 2018 at 12:27:23PM +0100, Vincenzo Frascino wrote:
On 03/09/18 16:10, Luc Van Oostenryck wrote:
On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
On 03/09/18 13:34, Andrey Konovalov wrote:
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote: > This patch adds __force annotations for __user pointers casts detected by > sparse with the -Wcast-from-as flag enabled (added in [1]). > > [1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
Hi,
It would be nice to have some explanation for why these added __force are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I think as well that we should make explicit the information that __force is hiding. A possible solution could be defining some new address spaces and use them where it is relevant in the kernel. Something like:
# define __compat_ptr __attribute__((noderef, address_space(5))) # define __tagged_ptr __attribute__((noderef, address_space(6)))
In this way sparse can still identify the casting and trigger a warning.
We could at that point modify sparse to ignore these conversions when a specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr) to exclude from the generated warnings the ones we have already dealt with.
What do you think about this approach?
I'll be happy to add such warnings to sparse if it is useful to detect (and correct!) problems. I'm also thinking to other possiblities, like having some weaker form of __force (maybe simply __force_as (which will 'only' force the address space) or even __force_as(TO, FROM) (with TO and FROM being a mask of the address space allowed).I believe we need something here to address this type of problems and I like
your proposal of adding a weaker force in the form of __force_as(TO, FROM) because I think it provides the right level information.
However, for the specific situation here, I'm not sure that using address spaces is the right choice because I suspect that the concept of tagged pointer is orthogonal to the one of (the usual) address space (it won't be possible for a pointer to be __tagged_ptr *and* __user).
I was thinking to address spaces because the information seems easily accessible in sparse [1], but I am certainly open to any solution that can be semantically more correct.
Yes, adding a new address space is easy (and doesn't need any modification to sparse). Here, I think adding a new 'modifier' __tagged (much like __nocast, __noderef, ...) would be much more appropriate. I think that at this point, it would be nice to have a clear description of the problem and what sort of checks are wanted.
OTOH, when I see already the tons of warnings for concepts established since many years (I'm thinking especially at __bitwise, see [1]) I'm a bit affraid of adding new, more specialized ones that people will understand even less how/when they need to use them.
Thanks for providing this statistic, it is very interesting. I understand your concern, but I think that in this case we need a more specialized option not only to find potential problems but even to provide the right amount of information to who reads the code.
A solution could be to let __force_as(TO, FROM) behave like __force and silence the warning by default, but have an option in sparse to re-enable it (i.e. -Wshow-force-as).
That would be, indeed, a simple solution but IMO even more dangerous than __force itself (since by readingthe code with this annotation people would naturally think it only involves the AS will in fact it would be the same as __force). I prefer to directly implement a plain __force_as, forcing only the AS).
[1]
commit ee7985f0c2b29c96aefe78df4139209eb4e719d8 Author: Vincenzo Frascino vincenzo.frascino@arm.com Date: Wed Aug 15 10:55:44 2018 +0100
print address space number for explicit cast to ulongThis patch build on top of commit b34880d ("stricter warning for explicit cast to ulong") and prints the address space number when a "warning: cast removes address space of expression" is triggered. This makes easier to discriminate in between different address spaces. A validation example is provided as well as part of this patch. Signed-off-by: Vincenzo Frascino vincenzo.frascino@arm.com
diff --git a/evaluate.c b/evaluate.c index 6d5d479..2fc0ebc 100644 --- a/evaluate.c +++ b/evaluate.c @@ -3017,8 +3017,12 @@ static struct symbol *evaluate_cast(struct expression *expr) sas = stype->ctype.as; }
- if (!tas && sas > 0)
warning(expr->pos, "cast removes address space of expression");
- if (!tas && sas > 0) {
if (Wcast_from_as)warning(expr->pos, "cast removes address space of expression (<asn:%d>)", sas);elsewarning(expr->pos, "cast removes address space of expression");- }
I think that the if (Wcast_from_as) is unneeded, the asn:%d can be added even if Wcast_from_as is false. Woukd it be OK for you?
-- Luc
On 05/09/18 20:03, Luc Van Oostenryck wrote:
On Tue, Sep 04, 2018 at 12:27:23PM +0100, Vincenzo Frascino wrote:
On 03/09/18 16:10, Luc Van Oostenryck wrote:
On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
On 03/09/18 13:34, Andrey Konovalov wrote:
On Fri, Aug 31, 2018 at 3:42 PM, Al Viro viro@zeniv.linux.org.uk wrote:
On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote: > On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote: >> This patch adds __force annotations for __user pointers casts detected by >> sparse with the -Wcast-from-as flag enabled (added in [1]). >> >> [1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e... > > Hi, > > It would be nice to have some explanation for why these added __force > are useful.
I'll add this in the next version, thanks!
It would be even more useful if that series would either deal withthe noise for real ("that's what we intend here, that's what we intend there, here's a primitive for such-and-such kind of cases, here we actually ought to pass __user pointer instead of unsigned long", etc.) or left it unmasked.
As it is, __force says only one thing: "I know the code is doingthe right thing here". That belongs in primitives, and I do *not* mean the #define cast_to_ulong(x) ((__force unsigned long)(x)) kind.
Folks, if you don't want to deal with that - leave the warnings be.They do carry more information than "someone has slapped __force in that place".
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I think as well that we should make explicit the information that __force is hiding. A possible solution could be defining some new address spaces and use them where it is relevant in the kernel. Something like:
# define __compat_ptr __attribute__((noderef, address_space(5))) # define __tagged_ptr __attribute__((noderef, address_space(6)))
In this way sparse can still identify the casting and trigger a warning.
We could at that point modify sparse to ignore these conversions when a specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr) to exclude from the generated warnings the ones we have already dealt with.
What do you think about this approach?
I'll be happy to add such warnings to sparse if it is useful to detect (and correct!) problems. I'm also thinking to other possiblities, like having some weaker form of __force (maybe simply __force_as (which will 'only' force the address space) or even __force_as(TO, FROM) (with TO and FROM being a mask of the address space allowed).I believe we need something here to address this type of problems and I like
your proposal of adding a weaker force in the form of __force_as(TO, FROM) because I think it provides the right level information.
However, for the specific situation here, I'm not sure that using address spaces is the right choice because I suspect that the concept of tagged pointer is orthogonal to the one of (the usual) address space (it won't be possible for a pointer to be __tagged_ptr *and* __user).
I was thinking to address spaces because the information seems easily accessible in sparse [1], but I am certainly open to any solution that can be semantically more correct.
Yes, adding a new address space is easy (and doesn't need any modification to sparse). Here, I think adding a new 'modifier' __tagged (much like __nocast, __noderef, ...) would be much more appropriate. I think that at this point, it would be nice to have a clear description of the problem and what sort of checks are wanted.
The problem we are trying to address here is to identify when the user pointers are cast to integer types and to sanitize (when required) the kernel, when this happens.
The way on which we are trying to address this problem based on what Andrey proposed in his patch-set is to use the Top Byte Ignore feature (which is a 64 bit specific feature).
Based on what I said I think that we require 2 'modifiers': - __compat (or __compat_ptr) used when the kernel is dealing with user compat pointers (32 bit, they can not be tagged). It should behave like force (silence warnings), but having something separate IMO makes more clear the intention of what we are trying to do. - __tagged (or __tagged_ptr) used when the kernel is dealing with user normal pointers (which can be tagged). In this case sparse should still be able to trigger a warning (that can be disabled by default as I was proposing in my previous email). When we put a tagged identifier we declare that we analyzed the code impacted by the conversion and eventually sanitized it. Having the warning still there allows us or whoever is looking at the code to always go back to the identified issue.
OTOH, when I see already the tons of warnings for concepts established since many years (I'm thinking especially at __bitwise, see [1]) I'm a bit affraid of adding new, more specialized ones that people will understand even less how/when they need to use them.
Thanks for providing this statistic, it is very interesting. I understand your concern, but I think that in this case we need a more specialized option not only to find potential problems but even to provide the right amount of information to who reads the code.
A solution could be to let __force_as(TO, FROM) behave like __force and silence the warning by default, but have an option in sparse to re-enable it (i.e. -Wshow-force-as).
That would be, indeed, a simple solution but IMO even more dangerous than __force itself (since by readingthe code with this annotation people would naturally think it only involves the AS will in fact it would be the same as __force). I prefer to directly implement a plain __force_as, forcing only the AS).
Agreed, even if we can't stop people from overlooking at things, I believe that we will all benefit if they are clearer.
[1]
commit ee7985f0c2b29c96aefe78df4139209eb4e719d8 Author: Vincenzo Frascino vincenzo.frascino@arm.com Date: Wed Aug 15 10:55:44 2018 +0100
print address space number for explicit cast to ulongThis patch build on top of commit b34880d ("stricter warning for explicit cast to ulong") and prints the address space number when a "warning: cast removes address space of expression" is triggered. This makes easier to discriminate in between different address spaces. A validation example is provided as well as part of this patch. Signed-off-by: Vincenzo Frascino vincenzo.frascino@arm.com
diff --git a/evaluate.c b/evaluate.c index 6d5d479..2fc0ebc 100644 --- a/evaluate.c +++ b/evaluate.c @@ -3017,8 +3017,12 @@ static struct symbol *evaluate_cast(struct expression *expr) sas = stype->ctype.as; }
- if (!tas && sas > 0)
warning(expr->pos, "cast removes address space of expression");
- if (!tas && sas > 0) {
if (Wcast_from_as)warning(expr->pos, "cast removes address space of expression (<asn:%d>)", sas);elsewarning(expr->pos, "cast removes address space of expression");- }
I think that the if (Wcast_from_as) is unneeded, the asn:%d can be added even if Wcast_from_as is false. Woukd it be OK for you?
Yes, it is OK for me (I put it there because I did not know if we wanted to preserve the original behavior). Feel free to hack my patch if you want to put it on your tree. Thanks.
-- Luc
On Thu, Sep 06, 2018 at 03:13:16PM +0100, Vincenzo Frascino wrote:
On 05/09/18 20:03, Luc Van Oostenryck wrote:
I think that at this point, it would be nice to have a clear description of the problem and what sort of checks are wanted.
The problem we are trying to address here is to identify when the user pointers are cast to integer types and to sanitize (when required) the kernel, when this happens.
The way on which we are trying to address this problem based on what Andrey proposed in his patch-set is to use the Top Byte Ignore feature (which is a 64 bit specific feature).
Based on what I said I think that we require 2 'modifiers':
- __compat (or __compat_ptr) used when the kernel is dealing with user compat
pointers (32 bit, they can not be tagged). It should behave like force (silence warnings), but having something separate IMO makes more clear the intention of what we are trying to do.
- __tagged (or __tagged_ptr) used when the kernel is dealing with user normal
pointers (which can be tagged). In this case sparse should still be able to trigger a warning (that can be disabled by default as I was proposing in my previous email). When we put a tagged identifier we declare that we analyzed the code impacted by the conversion and eventually sanitized it. Having the warning still there allows us or whoever is looking at the code to always go back to the identified issue.
OK. Thanks for the explanation.
So, the way I see things from a type checking perspective, is that 'being (potentially) tagged' is a new property of values, othogonal the the concept of address space. Leaving the other address spaces (__iomem, __percpu & __rcu) aside, it should be possible to have __user & __kernel tagged pointers as well as tagged ulongs: __user __tagged * __kernel __tagged * ulong __tagged in addition of the usuals: __user * __kernel * ulong But some of them are banished or meaningless: __user * (all __user pointers are potentially tagged) __kernel __tagged * (tags are only for user space) ulong __tagged (pointers need to be untagged during conversion) So, only the followings remain: __user __tagged * __kernel * ulong and the property '__tagged' becomes equivalent to '__user'. Thus '__tagged' can be implicit and this would have the advantage of not needing to change any annotations.
Since the conversion '__user *' to '__kernel *' is already covered by the default sparse warnings, only the conversion '__user' to 'ulong' need to be covered (and this is already covered by the new option -Wcast-from-as) but that is only fine for detection. After detection and auditing, several solution are possible: 1) simply add '__force' in the cast (this is very bad) 2) moving this '__force' inside a macro '__untag_ptr(x)' would already more acceptable but is fundamentaly the same as 1) 3) a weaker form of '__force', '__force_as', will do the trick nicely as long as __user is equated to __tagged (and could be useful on its own but could also hide real AS conversion problems). 4) a more specific solution would be to effectively add a new attribute, '__tagged', to define '__user' like: #define __user attribute((noderef,address_space(1),tagged)) and have something like '__untag', a weaker form of __force meaning: "I know what I'm doing regarding conversion from 'tagged'".
Neither 3) nor 4) should be much work but while I firmly think that 4) is the most correct solution, I'm not sure it's worth the added complexity, certainly if KHWASAN is not meant to be upstreamed.
For the compat pointers, I'm less sure to understand the situation: even if they can't be tagged, treating them as the other __user pointers will still be OK (but I understand that it could be interesting to be able to track them, it's just that it's independent from the __tagged property).
-- Luc
On Mon, Sep 03, 2018 at 02:34:27PM +0200, Andrey Konovalov wrote:
Al, very annoyed by that kind of information-hiding crap...
This patch only adds __force to hide the reports I've looked at and decided that the code does the right thing. The cases where this is not the case are handled by the previous patches in the patchset. I'll this to the patch description as well. Is that OK?
I don't know about you, but personally I've run into "I've looked, I'm sure it's OK here" -> (a year or so later) "why is it OK, again? Oh, bugger..." quite a few times. Some, but not all, hadn't been OK all along, some used to be but got quietly broken by subsequent changes by people who had no idea that some property of implementation was critical for correctness in the place in question, some (even more embarrassingly) were broken by a patch of my own.
It happens. "Looked in there, decided that the warning was bogus and quietly shut it up" has turned out to be a source of trouble down the road a lot of times. If you are forcibly removing a warning (not by reorganizing the logics and annotations, that is - by force-cast, or something similar to it), leave behind something more useful than "On $DATE I've decided it was OK" (and even that - only accessible via git blame/git show). As a hint for yourself, if nothing else - you might end up asking yourself the same question a year or two (or twenty, for that matter) later while looking for likely source of odd breakage and trying to narrow the search down.
Force-cast conflates a *lot* of situations together - it's pretty much "fuck off, I know what's going on here, it's OK" and no more than that; hell, even the warning it removes would've carried more information...
That kind of "these are false positives, let's turn them off to search for real problems" patches is fine when developing a branch like that; it's leaving them in for posterity that tends to cause PITA...
I'm not attacking you, BTW - it's really a generic point re force-casts. There had been some really outrageous cases lately[1] and I think that this point does need to be made. Unexplained force-cast is worse than leaving a warning in.
[1] with, if my reading of the situation is correct, newbie asking maintainers if dealing with endianness warnings in a certain driver would be useful newbie getting told (perhaps by maintainers, perhaps by somebody else) that those were all noise, the driver's correct and the most useful thing to be done with them would be to make them STFU force-cast-laden patch from said newbie doing just that picked by said maintainers, "cleaning up" the warnings. And committed with authorship pinned to the newbie ;-/ Not nice, seeing that the code in driver is *not* correct, despite the high-handed "shut that noise off, everything's fine there" commit - undoing that "cleanup" and trying to redo annotations properly starts to converge on absolutely real bugs on b-e hosts in about 10 minutes. In PCIe driver, with devices existing as separate cards, not just something always embedded into x86 or arm motherboard...
On Thu, Aug 30, 2018 at 4:41 AM Andrey Konovalov andreyknvl@google.com wrote:
This patch adds __force annotations for __user pointers casts detected by sparse with the -Wcast-from-as flag enabled (added in [1]).
No, several of these are wrong, and just silence a warning that shows a problem.
So for example:
static inline compat_uptr_t ptr_to_compat(void __user *uptr) {
return (u32)(unsigned long)uptr;
return (u32)(__force unsigned long)uptr;}
this actually looks correct.
But:
--- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -76,7 +76,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si { unsigned long ret, limit = current_thread_info()->addr_limit;
__chk_user_ptr(addr);
__chk_user_ptr((void __force *)addr);
This looks actively wrong. The whole - and only - point of "__chk_user_ptr()" is that it warns about a lack of a "__user *" type.
So the above makes no sense at all.
There are other similar "that makes no sense what-so-ever", like this one:
struct compat_group_req __user *gr32 = (void *)optval;
struct compat_group_req __user *gr32 = (__force void *)optval;
no, the additionl of __force is not the right thing, the problem, is that a __user pointer is cast to a non-user 'void *' only to be assigned to another user type.
The fix should have been to use (void __user *) as the cast instead, no __force needed.
In general, I think the patch shows all the signs of "mindlessly just add casts", which is exactly the wrong thing to do to sparse warnings.
Linus
On Thu, Sep 6, 2018 at 2:13 PM Linus Torvalds torvalds@linux-foundation.org wrote:
So for example:
static inline compat_uptr_t ptr_to_compat(void __user *uptr) {
return (u32)(unsigned long)uptr;
return (u32)(__force unsigned long)uptr;}
this actually looks correct.
Side note: I do think that while the above is correct, the rest of the patch shows that we might be better off simply not havign the warning for address space changes at all for the "cast a pointer to an integer type" case.
When you cast to a non-pointer type, the address space issue simply doesn't exist at all, so the warning makes less sense.
It's really just he "pointer to one address space" being cast to "pointer to another address space" that should really warn, and that might need that "__force" thing.
Hmm? So maybe a sparse change is better for most of that patch.
Linus
On Thu, Sep 06, 2018 at 02:16:19PM -0700, Linus Torvalds wrote:
On Thu, Sep 6, 2018 at 2:13 PM Linus Torvalds torvalds@linux-foundation.org wrote:
So for example:
static inline compat_uptr_t ptr_to_compat(void __user *uptr) {
return (u32)(unsigned long)uptr;
return (u32)(__force unsigned long)uptr;}
this actually looks correct.
Side note: I do think that while the above is correct, the rest of the patch shows that we might be better off simply not havign the warning for address space changes at all for the "cast a pointer to an integer type" case.
When you cast to a non-pointer type, the address space issue simply doesn't exist at all, so the warning makes less sense.
It's really just he "pointer to one address space" being cast to "pointer to another address space" that should really warn, and that might need that "__force" thing.
Hmm? So maybe a sparse change is better for most of that patch.
Unless I'm misunderstanding something, I don't think there is anything to change for this specific point. Sparse don't warn (by default) on "cast from pointer with address space to integer", as it always been the case, I think. I think it's the good choice.
It's just that recently, I've added a new flag -Wcast-from-as [1], defaulting to 'no', specifically to *detect* these cast because of these tagged pointers.
Note: I tend to think more and more that __force is simply too strong and weaker form, like __force_as and __force_bitwise would be more appropriate.
-- Luc Van Oostenryck
[1] d96da358c ("stricter warning for explicit cast to ulong")
On Thu, Sep 06, 2018 at 02:16:19PM -0700, Linus Torvalds wrote:
On Thu, Sep 6, 2018 at 2:13 PM Linus Torvalds torvalds@linux-foundation.org wrote:
So for example:
static inline compat_uptr_t ptr_to_compat(void __user *uptr) {
return (u32)(unsigned long)uptr;
return (u32)(__force unsigned long)uptr;}
this actually looks correct.
Side note: I do think that while the above is correct, the rest of the patch shows that we might be better off simply not havign the warning for address space changes at all for the "cast a pointer to an integer type" case.
When you cast to a non-pointer type, the address space issue simply doesn't exist at all, so the warning makes less sense.
That's actually a new (potential) issue introduced by these patches. The arm64 architecture has a feature called Top Byte Ignore (TBI, a.k.a. tagged pointers) where the top 8-bit of a 64-bit pointer can be set to anything and the hardware automatically ignores it when dereferencing. The arm64 user/kernel ABI currently mandates that any pointer passed from user space to the kernel must have the top byte 0.
This patchset is proposing to relax the ABI so that user pointers with a non-zero top byte can be actually passed via kernel syscalls. It basically moves the responsibility to remove the pointer tag (where needed) from user to the kernel (and for some good reasons, user space can't always do it given the way hwasan is implemented in LLVM).
The downside is that now a tagged user pointer may not represent just a virtual address but address|tag, so things like access_ok() or find_extended_vma() need to untag (clear the top byte of) the pointer before use. Note that copy_from_user() etc. can safely dereference a tagged user pointer as the tag is automatically ignored by the hardware.
The arm64 maintainers asked for a more reliable approach to identifying existing and new cases where such explicit untagging is required and one of the proposals was a sparse option. Based on some observations, it seems that untagging is needed when a pointer is cast to a long and the pointer tag information can be dropped. With the sparse patch, there are lots of warnings where we actually can preserve the tag (e.g. compat user pointers should be ignored since the top 32-bit are always 0), so Andrey is trying to mask such warnings out so that we can detect new potential issues as the kernel evolves.
So it's not about casting to another pointer; it's rather about no longer using the value as a user pointer but as an actual (untyped, untagged) virtual address.
There may be better options to address this but I haven't seen any concrete proposal so far. Or we could simply consider that we've found all places where it matters and not bother with any static analysis tools (but for the time being it's still worth investigating whether we can do better than this).
It's really just he "pointer to one address space" being cast to "pointer to another address space" that should really warn, and that might need that "__force" thing.
I think sparse already warns if changing the address space of a pointer.
On Fri, Sep 7, 2018 at 8:26 AM Catalin Marinas catalin.marinas@arm.com wrote:
So it's not about casting to another pointer; it's rather about no longer using the value as a user pointer but as an actual (untyped, untagged) virtual address.
There may be better options to address this but I haven't seen any concrete proposal so far. Or we could simply consider that we've found all places where it matters and not bother with any static analysis tools (but for the time being it's still worth investigating whether we can do better than this).
I actually originally wanted to have sparse not just check types, but actually do transformations too, in order to check more.
For example, for just the user pointer case, we actually have two wildy different kinds of user pointers: "checked" user pointers and "wild" user pointers.
Most of the time it doesn't matter, but it does for the unsafe ones: "__get_user()" and friends.
So long long ago I wanted sparse to not just do the completely static type analysis, but also do actual "data flow" analysis where doing an "access_on()" on a pointer would turn it from "wild" to "checked", and then I could have warned about "hey, this function does __get_user(), but the flow analysis shows that you passed it a pointer that had never been checked".
But sparse never ended up doing that kind of much smarter things. Some of the lock context stuff does it on a very small local level, and not very well there either.
But it sounds like this is exactly what you guys would want for the tagged pointers. Some functions can take a "wild" pointer, because they deal with the tag part natively. And others need to be "checked" and have gone through the cleaning and verification.
But sparse is sadly not the right tool for this, and having a single "__user" address space is not sufficient. I guess for the arm64 case, you really could make up a *new* address space: "__user_untagged", and then have functions that convert from "void __user *" to "void __user_untagged *", and then mark the functions that need the tag removed as taking that new kind of user pointer.
And if you never mix types, that would actually work. But I'm guessing you can also pass "__user_untagged" pointers to the regular user access functions, and you do?
Linus
Hi Linus,
On Fri, Sep 07, 2018 at 09:30:35AM -0700, Linus Torvalds wrote:
On Fri, Sep 7, 2018 at 8:26 AM Catalin Marinas catalin.marinas@arm.com wrote:
So it's not about casting to another pointer; it's rather about no longer using the value as a user pointer but as an actual (untyped, untagged) virtual address.
[...]
I actually originally wanted to have sparse not just check types, but actually do transformations too, in order to check more.
[...]
But it sounds like this is exactly what you guys would want for the tagged pointers. Some functions can take a "wild" pointer, because they deal with the tag part natively. And others need to be "checked" and have gone through the cleaning and verification.
But sparse is sadly not the right tool for this, and having a single "__user" address space is not sufficient. I guess for the arm64 case, you really could make up a *new* address space: "__user_untagged", and then have functions that convert from "void __user *" to "void __user_untagged *", and then mark the functions that need the tag removed as taking that new kind of user pointer.
Fortunately, most (all) functions taking a __user pointer can cope with tagged pointers since they never dereference the pointer directly but pass it through uaccess functions (which can access tagged pointers without untagging). The problem appears when the pointer is no longer used for access but converted to a long for other uses like rbtree look-up, so not actually dereferenced. Such conversion, in a few cases, needs to lose the tag.
Of course, there are lots of void __user * conversions to long where removing the tag is not always the right thing or required (hence the __force annotations in this patchset).
As Luc mentioned in this thread, we can consider that __user pointers are always tagged. What I think we'd need is a few annotations where ulong must be an __untagged address (and I guess in smaller numbers than the __force ones proposed here). For example we can allow get_user_pages() to get an (ulong)(void __user *) conversion but find_vma() would only take an (unsigned long __untagged) argument. Such attribute conversion would be handled by an untagged_addr() macro. So we move the detection problem from pointer conversion to an ulong (tagged by default) to ulong __untagged conversion (I'm not sure sparse can do this).
That's slightly different than trying to identify all the __user ptr to long conversions but, as you said, it's probably not a complete solution anyway and with lots of __force annotations throughout the kernel.
I took another look at the changes this patchset does to the kernel and here are my thoughts:
I see two ways how a (potentially tagged) user pointer gets into the kernel:
1. A pointer is passed to a syscall (directly as an argument or indirectly as a struct field). 2. A pointer is extracted from user context (registers, etc.) by some kind of a trap/fault handler. (Is there something else?)
In case 1 we also have a special case of a pointer passed to one of the memory syscalls (mmap, mprotect, etc.). These syscalls "are not doing memory accesses but rather dealing with the memory range, hence an untagged pointer is better suited" as pointed out by Catalin (these syscalls do not always use "unsigned long" instead of "void __user *" though, for example shmat uses "void __user *").
Looking at patch #8 ("usb, arm64: untag user addresses in devio") in this series, it seems that that devio ioctl actually accepts a pointer into a vma, so we shouldn't actually be untagging its argument and the patch needs to be dropped. Otherwise there's quite a few more cases that needs to be changed (like tcp_zerocopy_receive() for example, more can be found by grepping find_vma() in generic code).
Regarding case 2, it seems that analyzing casts of __user pointers won't really help, since the code (arch/arm64/mm/fault.c) doesn't really use them. However all of this code is arch specific, so it shouldn't really change over time (right?). It looks like dealing with tags passed to the kernel through these fault handlers is already resolved with these patches (and therefore patch #6 ("arm64: untag user address in __do_user_fault") in this series is not actually needed and can be dropped (need to test that)):
276e9327 ("arm64: entry: improve data abort handling of tagged pointers"), 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a tagged pointer") 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged pointers")
Now, I also see two cases when kernel behavior changes depending on whether a pointer is tagged:
1. Kernel code checks that a pointer belongs to userspace by comparing it with TASK_SIZE/addr_limit/user_addr_max()/USER_DS/... . 2. A pointer gets passed to find_vma() or similar functions. (Is there something else?)
The initial thought that I had here is that the pointers that reach find_vma() must be passed through memory syscalls and therefore shouldn't be untagged and don't require any fixes. There are at least two exceptions to this: 1. get_user_pages() (see patch #4 ("mm, arm64: untag user addresses in mm/gup.c") in this patch series) and 2. __do_page_fault() in arch/arm64/mm/fault.c. Are there any other obvious exceptions? I've tried adding BUG_ON(has_tag(addr)) to find_vma() and running a modified syzkaller version that passes tagged pointers to the kernel and failed to find anything else.
As for case 1, the places where pointers are compared with TASK_SIZE and others can be found with grep. Maybe it makes sense to introduce some kind of routine like is_user_pointer() that handles tagged pointers and refactor the existing code to use it? And maybe add a rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and others.
So I think detecting direct comparisons with TASK_SIZE and others would more useful than finding __user pointer casts (it seems that the latter requires a lot of annotations to be fixed/added), and I should just drop this patch with annotations.
WDYT?
On Mon, Sep 17, 2018 at 7:01 PM, Andrey Konovalov andreyknvl@google.com wrote:
I took another look at the changes this patchset does to the kernel and here are my thoughts:
I see two ways how a (potentially tagged) user pointer gets into the kernel:
- A pointer is passed to a syscall (directly as an argument or
indirectly as a struct field). 2. A pointer is extracted from user context (registers, etc.) by some kind of a trap/fault handler. (Is there something else?)
In case 1 we also have a special case of a pointer passed to one of the memory syscalls (mmap, mprotect, etc.). These syscalls "are not doing memory accesses but rather dealing with the memory range, hence an untagged pointer is better suited" as pointed out by Catalin (these syscalls do not always use "unsigned long" instead of "void __user *" though, for example shmat uses "void __user *").
Looking at patch #8 ("usb, arm64: untag user addresses in devio") in this series, it seems that that devio ioctl actually accepts a pointer into a vma, so we shouldn't actually be untagging its argument and the patch needs to be dropped. Otherwise there's quite a few more cases that needs to be changed (like tcp_zerocopy_receive() for example, more can be found by grepping find_vma() in generic code).
Regarding case 2, it seems that analyzing casts of __user pointers won't really help, since the code (arch/arm64/mm/fault.c) doesn't really use them. However all of this code is arch specific, so it shouldn't really change over time (right?). It looks like dealing with tags passed to the kernel through these fault handlers is already resolved with these patches (and therefore patch #6 ("arm64: untag user address in __do_user_fault") in this series is not actually needed and can be dropped (need to test that)):
276e9327 ("arm64: entry: improve data abort handling of tagged pointers"), 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a tagged pointer") 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged pointers")
Now, I also see two cases when kernel behavior changes depending on whether a pointer is tagged:
- Kernel code checks that a pointer belongs to userspace by comparing
it with TASK_SIZE/addr_limit/user_addr_max()/USER_DS/... . 2. A pointer gets passed to find_vma() or similar functions. (Is there something else?)
The initial thought that I had here is that the pointers that reach find_vma() must be passed through memory syscalls and therefore shouldn't be untagged and don't require any fixes. There are at least two exceptions to this: 1. get_user_pages() (see patch #4 ("mm, arm64: untag user addresses in mm/gup.c") in this patch series) and 2. __do_page_fault() in arch/arm64/mm/fault.c. Are there any other obvious exceptions? I've tried adding BUG_ON(has_tag(addr)) to find_vma() and running a modified syzkaller version that passes tagged pointers to the kernel and failed to find anything else.
As for case 1, the places where pointers are compared with TASK_SIZE and others can be found with grep. Maybe it makes sense to introduce some kind of routine like is_user_pointer() that handles tagged pointers and refactor the existing code to use it? And maybe add a rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and others.
So I think detecting direct comparisons with TASK_SIZE and others would more useful than finding __user pointer casts (it seems that the latter requires a lot of annotations to be fixed/added), and I should just drop this patch with annotations.
WDYT?
ping
Hi Andrey,
(sorry for the delay)
On Mon, Sep 17, 2018 at 07:01:00PM +0200, Andrey Konovalov wrote:
I took another look at the changes this patchset does to the kernel and here are my thoughts:
I see two ways how a (potentially tagged) user pointer gets into the kernel:
- A pointer is passed to a syscall (directly as an argument or
indirectly as a struct field). 2. A pointer is extracted from user context (registers, etc.) by some kind of a trap/fault handler. (Is there something else?)
Not AFAICT.
In case 1 we also have a special case of a pointer passed to one of the memory syscalls (mmap, mprotect, etc.). These syscalls "are not doing memory accesses but rather dealing with the memory range, hence an untagged pointer is better suited" as pointed out by Catalin (these syscalls do not always use "unsigned long" instead of "void __user *" though, for example shmat uses "void __user *").
If it makes things any simpler, we could revisit this though it seems to me more consistent not to pass a tagged pointer back into the kernel when the original one was untagged (i.e. mmap vs munmap).
(if it wasn't for pointers in structures, I would have just left the problem entirely to the C library to do the untagging before calling the kernel)
Looking at patch #8 ("usb, arm64: untag user addresses in devio") in this series, it seems that that devio ioctl actually accepts a pointer into a vma, so we shouldn't actually be untagging its argument and the patch needs to be dropped.
You are right, the pointer seems to have originated from the kernel as already untagged (mmap() on the driver), so we would expect the user to pass it back an untagged pointer.
Otherwise there's quite a few more cases that needs to be changed (like tcp_zerocopy_receive() for example, more can be found by grepping find_vma() in generic code).
Yes, it's similar to the devio one.
Regarding case 2, it seems that analyzing casts of __user pointers won't really help, since the code (arch/arm64/mm/fault.c) doesn't really use them. However all of this code is arch specific, so it shouldn't really change over time (right?). It looks like dealing with tags passed to the kernel through these fault handlers is already resolved with these patches (and therefore patch #6 ("arm64: untag user address in __do_user_fault") in this series is not actually needed and can be dropped (need to test that)):
I'm less worried about (2) since, as you say, it's under the arch control and, even if it changes slightly over time, we can be aware of this.
Now, I also see two cases when kernel behavior changes depending on whether a pointer is tagged:
- Kernel code checks that a pointer belongs to userspace by comparing
it with TASK_SIZE/addr_limit/user_addr_max()/USER_DS/... . 2. A pointer gets passed to find_vma() or similar functions. (Is there something else?)
I think these are the main cases.
The initial thought that I had here is that the pointers that reach find_vma() must be passed through memory syscalls and therefore shouldn't be untagged and don't require any fixes. There are at least two exceptions to this: 1. get_user_pages() (see patch #4 ("mm, arm64: untag user addresses in mm/gup.c") in this patch series) and 2. __do_page_fault() in arch/arm64/mm/fault.c. Are there any other obvious exceptions?
Vincenzo F did some more in-depth analysis, I'll let him answer whether he has found any more cases.
At a quick grep, arm64_notify_segfault() (it changes the siginfo si_code). There are a few other places which assume that the address is already untagged but we don't seem to have a clear guideline on the ABI. For example, prctl(PR_SET_MM) takes user addresses and we assume they are untagged (as in the mmap() and friends).
I've tried adding BUG_ON(has_tag(addr)) to find_vma() and running a modified syzkaller version that passes tagged pointers to the kernel and failed to find anything else.
I added a similar test with an LD_PRELOAD'ed malloc/free implementation on Debian and seemed alright but I'd rather want something more consistent when defining the user ABI, otherwise we have places where find_vma() is called but requires untagging.
As for case 1, the places where pointers are compared with TASK_SIZE and others can be found with grep. Maybe it makes sense to introduce some kind of routine like is_user_pointer() that handles tagged pointers and refactor the existing code to use it? And maybe add a rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and others.
So I think detecting direct comparisons with TASK_SIZE and others would more useful than finding __user pointer casts (it seems that the latter requires a lot of annotations to be fixed/added), and I should just drop this patch with annotations.
I think point (1) is not too bad, usually found with grep.
As I've said in my previous reply, I kind of came to the same conclusion that searching __user pointer casts to long may not actually scale. If we could add an __untagged annotation to ulong where it matters (e.g. find_vma()), we could identify a ulong (default tagged) and annotate some of those.
However, this analysis on __user * casting was useful even if we don't end up using it. If we come up with a clearer definition of the ABI (which syscalls accept tagged pointers), we may conclude that the only places where untagging matters are a few find_vma() calls in the arch and mm code and can ignore the rest.
FTR, this code is a prerequisite for ARM's memory tagging hardware feature. Interestingly, I can't find SPARC ADI support dealing with this aspect at all. I suspect their user ABI does not allow tagged addresses passed to the kernel.
On Fri, Sep 28, 2018 at 7:50 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Sep 17, 2018 at 07:01:00PM +0200, Andrey Konovalov wrote:
Looking at patch #8 ("usb, arm64: untag user addresses in devio") in this series, it seems that that devio ioctl actually accepts a pointer into a vma, so we shouldn't actually be untagging its argument and the patch needs to be dropped.
You are right, the pointer seems to have originated from the kernel as already untagged (mmap() on the driver), so we would expect the user to pass it back an untagged pointer.
OK, dropped this patch in v7.
As for case 1, the places where pointers are compared with TASK_SIZE and others can be found with grep. Maybe it makes sense to introduce some kind of routine like is_user_pointer() that handles tagged pointers and refactor the existing code to use it? And maybe add a rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and others.
So I think detecting direct comparisons with TASK_SIZE and others would more useful than finding __user pointer casts (it seems that the latter requires a lot of annotations to be fixed/added), and I should just drop this patch with annotations.
I think point (1) is not too bad, usually found with grep.
As I've said in my previous reply, I kind of came to the same conclusion that searching __user pointer casts to long may not actually scale. If we could add an __untagged annotation to ulong where it matters (e.g. find_vma()), we could identify a ulong (default tagged) and annotate some of those.
However, this analysis on __user * casting was useful even if we don't end up using it. If we come up with a clearer definition of the ABI (which syscalls accept tagged pointers), we may conclude that the only places where untagging matters are a few find_vma() calls in the arch and mm code and can ignore the rest.
So what exactly should I do now?
For now I've posted v7 with the sparse annotation patch dropped (to have the most up-do-date version posted).
On Thu, Aug 30, 2018 at 1:41 PM, Andrey Konovalov andreyknvl@google.com wrote:
arm64 has a feature called Top Byte Ignore, which allows to embed pointer tags into the top byte of each pointer. Userspace programs (such as HWASan, a memory debugging tool [1]) might use this feature and pass tagged user pointers to the kernel through syscalls or other interfaces.
This patch makes a few of the kernel interfaces accept tagged user pointers. The kernel is already able to handle user faults with tagged pointers and has the untagged_addr macro, which this patchset reuses.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Changes in v6:
- Added annotations for user pointer casts found by sparse.
Hi Catalin,
I've added annotations for the user pointer casts pointed by the new sparse flag -Wcast-from-as as you asked. I've used __force casts instead of adding specialized macros.
There are also non annotated casts for other pointer types (iomem, rcu) which are detected with the new flag, should I annotate those as well?
I'm not sure though what value would that bring though, as there are ~3000 various sparse warnings produced with the default flags anyway.
WDYT?
Thanks!
[1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e...
linux-kselftest-mirror@lists.linaro.org
-
Al Viro -
Andrey Konovalov -
Catalin Marinas -
Linus Torvalds -
Luc Van Oostenryck -
Vincenzo Frascino