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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
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 (7): 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: update Documentation/arm64/tagged-pointers.txt selftests, arm64: add a selftest for passing tagged pointers to kernel
Documentation/arm64/tagged-pointers.txt | 5 +++-- arch/arm64/include/asm/uaccess.h | 14 +++++++++----- include/linux/uaccess.h | 4 ++++ lib/strncpy_from_user.c | 2 ++ lib/strnlen_user.c | 2 ++ mm/gup.c | 4 ++++ 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 +++++++++++++++++++ 10 files changed, 67 insertions(+), 7 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 (like the mm subsystem), the untagged_addr macro should 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). Here we also need to handle the case of tagged user pointers.
Add untagging to gup.c functions that use user pointers 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 b70d7ba7cc13..5bb351c91989 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; int 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)) {
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; +}
On Wed, Jun 20, 2018 at 5:24 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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Hi!
Is there anything I should do to move forward with this?
I've received zero replies to this patch set (v3 and v4) over the last month.
Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrey,
On Tue, Jun 26, 2018 at 02:47:50PM +0200, Andrey Konovalov wrote:
On Wed, Jun 20, 2018 at 5:24 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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Is there anything I should do to move forward with this?
I've received zero replies to this patch set (v3 and v4) over the last month.
The patches in this series look fine but my concern is that they are not sufficient and we don't have (yet?) a way to identify where such annotations are required. You even say in patch 6 that this is "some initial work for supporting non-zero address tags passed to the kernel". Unfortunately, merging (or relaxing) an ABI without a clear picture is not really feasible.
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
Thanks.
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
Hi Andrey,
On Tue, Jun 26, 2018 at 02:47:50PM +0200, Andrey Konovalov wrote:
On Wed, Jun 20, 2018 at 5:24 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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Is there anything I should do to move forward with this?
I've received zero replies to this patch set (v3 and v4) over the last month.
The patches in this series look fine but my concern is that they are not sufficient and we don't have (yet?) a way to identify where such annotations are required. You even say in patch 6 that this is "some initial work for supporting non-zero address tags passed to the kernel". Unfortunately, merging (or relaxing) an ABI without a clear picture is not really feasible.
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/06/2018 16:05, Andrey Konovalov wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
Hi Andrey,
On Tue, Jun 26, 2018 at 02:47:50PM +0200, Andrey Konovalov wrote:
On Wed, Jun 20, 2018 at 5:24 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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Is there anything I should do to move forward with this?
I've received zero replies to this patch set (v3 and v4) over the last month.
The patches in this series look fine but my concern is that they are not sufficient and we don't have (yet?) a way to identify where such annotations are required. You even say in patch 6 that this is "some initial work for supporting non-zero address tags passed to the kernel". Unfortunately, merging (or relaxing) an ABI without a clear picture is not really feasible.
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
This sounds like the kind of thing we should be able to get sparse to do already, no ? It's been many years since I last looked at it but I thought sparse was the tool of choice in the kernel to do this kind of checking.
regards Ramana
Thanks!
-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 04:08:09PM +0100, Ramana Radhakrishnan wrote:
On 27/06/2018 16:05, Andrey Konovalov wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Tue, Jun 26, 2018 at 02:47:50PM +0200, Andrey Konovalov wrote:
On Wed, Jun 20, 2018 at 5:24 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.
We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
Is there anything I should do to move forward with this?
I've received zero replies to this patch set (v3 and v4) over the last month.
The patches in this series look fine but my concern is that they are not sufficient and we don't have (yet?) a way to identify where such annotations are required. You even say in patch 6 that this is "some initial work for supporting non-zero address tags passed to the kernel". Unfortunately, merging (or relaxing) an ABI without a clear picture is not really feasible.
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
This sounds like the kind of thing we should be able to get sparse to do already, no ? It's been many years since I last looked at it but I thought sparse was the tool of choice in the kernel to do this kind of checking.
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
Hi,
sparse doesn't have such attribute but would an new option that would warn on such cast be a solution for your case?
-- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
sparse doesn't have such attribute but would an new option that would warn on such cast be a solution for your case?
I can't tell for sure whether such sparse option would be the full solution but detecting explicit __user pointer casts to long is a good starting point. So far this patchset pretty much relies on detecting a syscall failure and trying to figure out why, patching the kernel. It doesn't really scale.
As a side note, we have cases in the user-kernel ABI where the user address type is "unsigned long": mmap() and friends. My feedback on an early version of this patchset was to always require untagged pointers coming from user space on such syscalls, so no need for explicit untagging.
On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
sparse doesn't have such attribute but would an new option that would warn on such cast be a solution for your case?
I can't tell for sure whether such sparse option would be the full solution but detecting explicit __user pointer casts to long is a good starting point. So far this patchset pretty much relies on detecting a syscall failure and trying to figure out why, patching the kernel. It doesn't really scale.
OK, I'll add such an option this evening.
As a side note, we have cases in the user-kernel ABI where the user address type is "unsigned long": mmap() and friends. My feedback on an early version of this patchset was to always require untagged pointers coming from user space on such syscalls, so no need for explicit untagging.
Mmmm yes. I tend to favor a sort of opposite approach. When we have an address that must not be dereferenced as-such (and sometimes when the address can be from both __user & __kernel space) I prefer to use a ulong which will force the use of the required operation before being able to do any sort of dereferencing and this won't need horrible casts with __force (it, of course, all depends on the full context).
-- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
sparse doesn't have such attribute but would an new option that would warn on such cast be a solution for your case?
I can't tell for sure whether such sparse option would be the full solution but detecting explicit __user pointer casts to long is a good starting point. So far this patchset pretty much relies on detecting a syscall failure and trying to figure out why, patching the kernel. It doesn't really scale.
OK, I'll add such an option this evening.
That's great, thanks. I think this should cover casting pointers to any integer types, not just "unsigned long" (e.g. long long).
The only downside is that with this patchset the untagging can be done after the conversion to ulong (get_user_pages()) as that's where the problem was noticed. With a new sparse feature, we'd have to annotate the conversion sites (not sure how many until we run the tool though).
As a side note, we have cases in the user-kernel ABI where the user address type is "unsigned long": mmap() and friends. My feedback on an early version of this patchset was to always require untagged pointers coming from user space on such syscalls, so no need for explicit untagging.
Mmmm yes. I tend to favor a sort of opposite approach. When we have an address that must not be dereferenced as-such (and sometimes when the address can be from both __user & __kernel space) I prefer to use a ulong which will force the use of the required operation before being able to do any sort of dereferencing and this won't need horrible casts with __force (it, of course, all depends on the full context).
I agree. That's what the kernel uses in functions like get_user_pages() which take ulong as an argument. Similarly mmap() and friends don't expect the pointer to be dereferenced, hence the ulong argument. The interesting part that the man page (and the C library header declaration) shows such address argument as void *. We could add a syscall wrapper in the arch code, only that it doesn't feel consistent with the "rule" that ulong addresses are not actually tagged pointers.
On Thu, Jun 28, 2018 at 03:48:59PM +0100, Catalin Marinas wrote:
On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
sparse is indeed an option. The current implementation doesn't warn on an explicit cast from (void __user *) to (unsigned long) since that's a valid thing in the kernel. I couldn't figure out if there's any other __attribute__ that could be used to warn of such conversion.
sparse doesn't have such attribute but would an new option that would warn on such cast be a solution for your case?
I can't tell for sure whether such sparse option would be the full solution but detecting explicit __user pointer casts to long is a good starting point. So far this patchset pretty much relies on detecting a syscall failure and trying to figure out why, patching the kernel. It doesn't really scale.
OK, I'll add such an option this evening.
That's great, thanks. I think this should cover casting pointers to any integer types, not just "unsigned long" (e.g. long long).
Yes, of course.
The only downside is that with this patchset the untagging can be done after the conversion to ulong (get_user_pages()) as that's where the problem was noticed. With a new sparse feature, we'd have to annotate the conversion sites (not sure how many until we run the tool though).
I've done a lot of hacking on sparse and as such I run a lot of tests. What I see with these tests is that a lot of annotations are wrong or missing so I'm very reluctant to add another attribute. Even more so one that would be only slightly different than an existing one. OTOH, if it's something localized or a one-shot and there is a need, ... why not?
What would you ideally need?
-- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Catalin Marinas
Sent: 28 June 2018 15:49
...
Mmmm yes. I tend to favor a sort of opposite approach. When we have an address that must not be dereferenced as-such (and sometimes when the address can be from both __user & __kernel space) I prefer to use a ulong which will force the use of the required operation before being able to do any sort of dereferencing and this won't need horrible casts with __force (it, of course, all depends on the full context).
I agree. That's what the kernel uses in functions like get_user_pages() which take ulong as an argument. Similarly mmap() and friends don't expect the pointer to be dereferenced, hence the ulong argument. The interesting part that the man page (and the C library header declaration) shows such address argument as void *. We could add a syscall wrapper in the arch code, only that it doesn't feel consistent with the "rule" that ulong addresses are not actually tagged pointers.
For most modern calling conventions it would make sense to put 'user' addresses (and physical ones from that matter) into a structure. That way you get much stronger typing from C itself.
The patch would, of course, be huge!
David
-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
sparse issues a warning when user pointers are casted to integer types except to unsigned longs which are explicitly allowed. However it may happen that we would like to also be warned on casts to unsigned long.
Fix this by adding a new warning flag: -Wcast-from-as (to mirrors -Wcast-to-as) which extends -Waddress-space to all casts that remove an address space attribute (without using __force).
References: https://lore.kernel.org/lkml/20180628102741.vk6vphfinlj3lvhv@armageddon.camb... Signed-off-by: Luc Van Oostenryck luc.vanoostenryck@gmail.com ---
This patch is available in the Git repository at: git://github.com/lucvoo/sparse-dev.git warn-cast-from-as
---------------------------------------------------------------- Luc Van Oostenryck (1): stricter warning for explicit cast to ulong
evaluate.c | 4 +-- lib.c | 2 ++ lib.h | 1 + sparse.1 | 9 ++++++ validation/Waddress-space-strict.c | 56 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 validation/Waddress-space-strict.c
diff --git a/evaluate.c b/evaluate.c index 194b97218..64e1067ce 100644 --- a/evaluate.c +++ b/evaluate.c @@ -2998,14 +2998,14 @@ static struct symbol *evaluate_cast(struct expression *expr) } }
- if (ttype == &ulong_ctype) + if (ttype == &ulong_ctype && !Wcast_from_as) tas = -1; else if (tclass == TYPE_PTR) { examine_pointer_target(ttype); tas = ttype->ctype.as; }
- if (stype == &ulong_ctype) + if (stype == &ulong_ctype && !Wcast_from_as) sas = -1; else if (sclass == TYPE_PTR) { examine_pointer_target(stype); diff --git a/lib.c b/lib.c index 308f8f699..0bb5232ab 100644 --- a/lib.c +++ b/lib.c @@ -248,6 +248,7 @@ static struct token *pre_buffer_end = NULL; int Waddress = 0; int Waddress_space = 1; int Wbitwise = 1; +int Wcast_from_as = 0; int Wcast_to_as = 0; int Wcast_truncate = 1; int Wconstexpr_not_const = 0; @@ -678,6 +679,7 @@ static const struct flag warnings[] = { { "address", &Waddress }, { "address-space", &Waddress_space }, { "bitwise", &Wbitwise }, + { "cast-from-as", &Wcast_from_as }, { "cast-to-as", &Wcast_to_as }, { "cast-truncate", &Wcast_truncate }, { "constexpr-not-const", &Wconstexpr_not_const}, diff --git a/lib.h b/lib.h index b0453bb6e..46e685421 100644 --- a/lib.h +++ b/lib.h @@ -137,6 +137,7 @@ extern int preprocess_only; extern int Waddress; extern int Waddress_space; extern int Wbitwise; +extern int Wcast_from_as; extern int Wcast_to_as; extern int Wcast_truncate; extern int Wconstexpr_not_const; diff --git a/sparse.1 b/sparse.1 index 806fb0cf0..62956f18b 100644 --- a/sparse.1 +++ b/sparse.1 @@ -77,6 +77,15 @@ Sparse issues these warnings by default. To turn them off, use \fB-Wno-bitwise\fR. . .TP +.B -Wcast-from-as +Warn about which remove an address space to a pointer type. + +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. +. +.TP .B -Wcast-to-as Warn about casts which add an address space to a pointer type.
diff --git a/validation/Waddress-space-strict.c b/validation/Waddress-space-strict.c new file mode 100644 index 000000000..ad23f74ae --- /dev/null +++ b/validation/Waddress-space-strict.c @@ -0,0 +1,56 @@ +#define __user __attribute__((address_space(1))) + +typedef unsigned long ulong; +typedef long long llong; +typedef struct s obj_t; + +static void expl(int i, ulong u, llong l, void *v, obj_t *o, obj_t __user *p) +{ + (obj_t*)(i); + (obj_t __user*)(i); + + (obj_t*)(u); + (obj_t __user*)(u); + + (obj_t*)(l); + (obj_t __user*)(l); + + (obj_t*)(v); + (obj_t __user*)(v); + + (int)(o); + (ulong)(o); + (llong)(o); + (void *)(o); + (obj_t*)(o); + (obj_t __user*)(o); + + (int)(p); // w + (ulong)(p); // w! + (llong)(p); // w + (void *)(p); // w + (obj_t*)(p); // w + (obj_t __user*)(p); // ok +} + +/* + * check-name: Waddress-space-strict + * check-command: sparse -Wcast-from-as -Wcast-to-as $file + * + * check-error-start +Waddress-space-strict.c:10:10: warning: cast adds address space to expression (asn:1) +Waddress-space-strict.c:13:10: warning: cast adds address space to expression (asn:1) +Waddress-space-strict.c:16:10: warning: cast adds address space to expression (asn:1) +Waddress-space-strict.c:19:10: warning: cast adds address space to expression (asn:1) +Waddress-space-strict.c:26:10: warning: cast adds address space to expression (asn:1) +Waddress-space-strict.c:28:10: warning: cast removes address space of expression +Waddress-space-strict.c:29:10: warning: cast removes address space of expression +Waddress-space-strict.c:30:10: warning: cast removes address space of expression +Waddress-space-strict.c:31:10: warning: cast removes address space of expression +Waddress-space-strict.c:32:10: warning: cast removes address space of expression +Waddress-space-strict.c:9:10: warning: non size-preserving integer to pointer cast +Waddress-space-strict.c:10:10: warning: non size-preserving integer to pointer cast +Waddress-space-strict.c:21:10: warning: non size-preserving pointer to integer cast +Waddress-space-strict.c:28:10: warning: non size-preserving pointer to integer cast + * check-error-end + */
On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
I've prototyped a checker on top of clang static analyzer (initially looked at sparse, but couldn't find any documentation or examples). The results are here [1], search for "warning: user pointer cast". Sharing in case anybody wants to take a look, will look at them myself tomorrow.
[1] https://gist.github.com/xairy/433edd5c86456a64026247cb2fef2115 -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
a bunch of compat a bunch of ioctl that use ptr to stored ints
ipc/shm.c:1355 ipc/shm.c:1566
mm/process_vm_access.c:178:20 mm/process_vm_access.c:180:19 substraction => harmless
mm/process_vm_access.c:221:4 ?
mm/memory.c:4679:14 should be __user pointer
fs/fuse/file.c:1256:9 ?
kernel/kthread.c:73:9 ?
mm/migrate.c:1586:10 mm/migrate.c:1660:24
lib/iov_iter.c ???
kernel/futex.c:502 uses user addr as key
kernel/futex.c:730 gup, fixed
lib/strncpy_from_user.c:110:13 fixed?
lib/strnlen_user.c:112 fixed?
fs/readdir.c:369 ???
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
I've prototyped a checker on top of clang static analyzer (initially looked at sparse, but couldn't find any documentation or examples). The results are here [1], search for "warning: user pointer cast". Sharing in case anybody wants to take a look, will look at them myself tomorrow.
[1] https://gist.github.com/xairy/433edd5c86456a64026247cb2fef2115
-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 5:19 PM, Andrey Konovalov andreyknvl@google.com wrote:
a bunch of compat a bunch of ioctl that use ptr to stored ints
ipc/shm.c:1355 ipc/shm.c:1566
mm/process_vm_access.c:178:20 mm/process_vm_access.c:180:19 substraction => harmless
mm/process_vm_access.c:221:4 ?
mm/memory.c:4679:14 should be __user pointer
fs/fuse/file.c:1256:9 ?
kernel/kthread.c:73:9 ?
mm/migrate.c:1586:10 mm/migrate.c:1660:24
lib/iov_iter.c ???
kernel/futex.c:502 uses user addr as key
kernel/futex.c:730 gup, fixed
lib/strncpy_from_user.c:110:13 fixed?
lib/strnlen_user.c:112 fixed?
fs/readdir.c:369 ???
Started looking at the results and accidentally posted my notes. Ignore this for now, will post when done. -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
I've prototyped a checker on top of clang static analyzer (initially looked at sparse, but couldn't find any documentation or examples). The results are here [1], search for "warning: user pointer cast". Sharing in case anybody wants to take a look, will look at them myself tomorrow.
[1] https://gist.github.com/xairy/433edd5c86456a64026247cb2fef2115
So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
Place 1:
arch/arm64/mm/fault.c:302:34: warning: user pointer cast current->thread.fault_address = (unsigned long)info->si_addr;
Compare a pointer with TASK_SIZE (1 << 48) to check whether it lies in the kernel or in user space. Need to untag the address before performing a comparison.
Place 2:
fs/namespace.c:2736:21: warning: user pointer cast size = TASK_SIZE - (unsigned long)data;
A similar check performed by subtracting a pointer from TASK_SIZE. Need to untag before subtracting.
Place 3:
drivers/usb/core/devio.c:1407:29: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer; drivers/usb/core/devio.c:1636:31: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer; drivers/usb/core/devio.c:1715:30: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer;
The device keeps list of mmapped areas and searches them for provided __user pointer. Need to untag before searching.
There are also a few cases of memory syscalls operating on __user pointers instead of unsigned longs like mmap:
ipc/shm.c:1355:23: warning: user pointer cast unsigned long addr = (unsigned long)shmaddr; ipc/shm.c:1566:23: warning: user pointer cast unsigned long addr = (unsigned long)shmaddr; mm/migrate.c:1586:10: warning: user pointer cast addr = (unsigned long)p; mm/migrate.c:1660:24: warning: user pointer cast unsigned long addr = (unsigned long)(*pages);
If we don't add untagging to mmap, we probably don't need it here.
The rest of reported places look fine as is. Full annotated results of running the checker are here [2].
I'll add the 3 patches with fixes to v5 of this patchset.
Catalin, WDYT?
[2] https://gist.github.com/xairy/aabda57741919df67d79895356ba9b58 -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 1:25 PM, Andrey Konovalov andreyknvl@google.com wrote:
So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
Place 1:
arch/arm64/mm/fault.c:302:34: warning: user pointer cast current->thread.fault_address = (unsigned long)info->si_addr;
Compare a pointer with TASK_SIZE (1 << 48) to check whether it lies in the kernel or in user space. Need to untag the address before performing a comparison.
Place 2:
fs/namespace.c:2736:21: warning: user pointer cast size = TASK_SIZE - (unsigned long)data;
A similar check performed by subtracting a pointer from TASK_SIZE. Need to untag before subtracting.
Place 3:
drivers/usb/core/devio.c:1407:29: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer; drivers/usb/core/devio.c:1636:31: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer; drivers/usb/core/devio.c:1715:30: warning: user pointer cast unsigned long uurb_start = (unsigned long)uurb->buffer;
The device keeps list of mmapped areas and searches them for provided __user pointer. Need to untag before searching.
There are also a few cases of memory syscalls operating on __user pointers instead of unsigned longs like mmap:
ipc/shm.c:1355:23: warning: user pointer cast unsigned long addr = (unsigned long)shmaddr; ipc/shm.c:1566:23: warning: user pointer cast unsigned long addr = (unsigned long)shmaddr; mm/migrate.c:1586:10: warning: user pointer cast addr = (unsigned long)p; mm/migrate.c:1660:24: warning: user pointer cast unsigned long addr = (unsigned long)(*pages);
If we don't add untagging to mmap, we probably don't need it here.
The rest of reported places look fine as is. Full annotated results of running the checker are here [2].
I'll add the 3 patches with fixes to v5 of this patchset.
Catalin, WDYT?
ping -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas catalin.marinas@arm.com wrote:
While I support this work, as a maintainer I'd like to understand whether we'd be in a continuous chase of ABI breaks with every kernel release or we have a better way to identify potential issues. Is there any way to statically analyse conversions from __user ptr to long for example? Or, could we get the compiler to do this for us?
OK, got it, I'll try to figure out a way to find these conversions.
I've prototyped a checker on top of clang static analyzer (initially looked at sparse, but couldn't find any documentation or examples). The results are here [1], search for "warning: user pointer cast". Sharing in case anybody wants to take a look, will look at them myself tomorrow.
[1] https://gist.github.com/xairy/433edd5c86456a64026247cb2fef2115
So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
[...]
I'll add the 3 patches with fixes to v5 of this patchset.
Thanks for investigating. You can fix those three places in your code but I was rather looking for a way to check such casting in the future for newly added code. While for the khwasan we can assume it's a debug option, the tagged user pointers are ABI and we need to keep it stable.
We could we actually add some macros for explicit conversion between __user ptr and long and silence the warning there (I guess this would work better for sparse). We can then detect new ptr to long casts as they appear. I just hope that's not too intrusive.
(I haven't tried the sparse patch yet, hopefully sometime this week)
On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote: So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
[...]
I'll add the 3 patches with fixes to v5 of this patchset.
Thanks for investigating. You can fix those three places in your code
OK, will do.
but I was rather looking for a way to check such casting in the future for newly added code. While for the khwasan we can assume it's a debug option, the tagged user pointers are ABI and we need to keep it stable.
We could we actually add some macros for explicit conversion between __user ptr and long and silence the warning there (I guess this would work better for sparse). We can then detect new ptr to long casts as they appear. I just hope that's not too intrusive.
(I haven't tried the sparse patch yet, hopefully sometime this week)
Haven't look at that sparse patch yet myself, but sounds doable. Should these macros go into this patchset or should they go separately? -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 2, 2018 at 5:00 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote: So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
[...]
I'll add the 3 patches with fixes to v5 of this patchset.
Thanks for investigating. You can fix those three places in your code
OK, will do.
but I was rather looking for a way to check such casting in the future for newly added code. While for the khwasan we can assume it's a debug option, the tagged user pointers are ABI and we need to keep it stable.
We could we actually add some macros for explicit conversion between __user ptr and long and silence the warning there (I guess this would work better for sparse). We can then detect new ptr to long casts as they appear. I just hope that's not too intrusive.
(I haven't tried the sparse patch yet, hopefully sometime this week)
Haven't look at that sparse patch yet myself, but sounds doable. Should these macros go into this patchset or should they go separately?
Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it? -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2018 at 04:59:18PM +0200, Andrey Konovalov wrote:
On Thu, Aug 2, 2018 at 5:00 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote: So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
[...]
I'll add the 3 patches with fixes to v5 of this patchset.
Thanks for investigating. You can fix those three places in your code
OK, will do.
but I was rather looking for a way to check such casting in the future for newly added code. While for the khwasan we can assume it's a debug option, the tagged user pointers are ABI and we need to keep it stable.
We could we actually add some macros for explicit conversion between __user ptr and long and silence the warning there (I guess this would work better for sparse). We can then detect new ptr to long casts as they appear. I just hope that's not too intrusive.
(I haven't tried the sparse patch yet, hopefully sometime this week)
Haven't look at that sparse patch yet myself, but sounds doable. Should these macros go into this patchset or should they go separately?
Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it?
Try using a more up-to-date version of sparse. Odds are you are using an old one, there is a newer version in a different branch on kernel.org somewhere...
greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2018 at 05:09:45PM +0200, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 04:59:18PM +0200, Andrey Konovalov wrote:
Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it?
Try using a more up-to-date version of sparse. Odds are you are using an old one, there is a newer version in a different branch on kernel.org somewhere...
That's not true. Building the current version of sparse from git://git.kernel.org/pub/scm/devel/sparse/sparse.git leaves me with a thousand errors just building the mm/ directory. A sample:
../mm/filemap.c:2353:21: warning: expression using sizeof(void) ../mm/filemap.c:2618:35: warning: symbol 'generic_file_vm_ops' was not declared. Should it be static? ../include/linux/slab.h:666:13: error: undefined identifier '__builtin_mul_overflow' ../include/linux/slab.h:666:13: warning: call with no type! ../include/linux/rcupdate.h:683:9: warning: context imbalance in 'find_lock_task_mm' - wrong count at exit ../include/linux/sched/mm.h:141:37: warning: dereference of noderef expression ../mm/page_alloc.c:886:1: error: directive in argument list ../include/trace/events/vmscan.h:79:1: warning: cast from restricted gfp_t ../include/trace/events/vmscan.h:196:1: warning: too many warnings (ahem!) ../mm/mmap.c:137:9: warning: cast to non-scalar ../mm/mmap.c:137:9: warning: cast from non-scalar ../mm/page_vma_mapped.c:134:29: warning: Using plain integer as NULL pointer ../include/linux/slab.h:631:13: warning: call with no type!
Basically, nobody is fixing their shit. The only way that sparse output is useful is to log the warnings before your changes, log them afterwards and run diff. The worst offender (as in: fixing it would remove most of the warnings) is the new min()/max() macro:
ra->start = max_t(long, 0, offset - ra->ra_pages / 2);
produces that first warning at line 2353 of filemap.c. I have no idea if this is a sparse mistake or something it's genuinely warning us about, but the sparse warnings are pretty ineffectual because nobody's paying attention to them. -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 3, 2018 at 6:43 PM, Matthew Wilcox willy@infradead.org wrote:
On Fri, Aug 03, 2018 at 05:09:45PM +0200, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 04:59:18PM +0200, Andrey Konovalov wrote:
Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it?
Try using a more up-to-date version of sparse. Odds are you are using an old one, there is a newer version in a different branch on kernel.org somewhere...
That's not true. Building the current version of sparse from git://git.kernel.org/pub/scm/devel/sparse/sparse.git leaves me with a thousand errors just building the mm/ directory. A sample:
I'm running the one from https://github.com/lucvoo/sparse-dev which seems to be even more up to date. Defconfig on x86 gives me ~3000 warnings:
https://gist.github.com/xairy/8adace989f64462e18ffb5cb7d096b73 -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 3, 2018 at 5:09 PM, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Aug 03, 2018 at 04:59:18PM +0200, Andrey Konovalov wrote:
On Thu, Aug 2, 2018 at 5:00 PM, Andrey Konovalov andreyknvl@google.com wrote:
On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas catalin.marinas@arm.com wrote:
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov andreyknvl@google.com wrote: So the checker reports ~100 different places where a __user pointer being casted. I've looked through them and found 3 places where we need to add untagging. Source code lines below come from 4.18-rc2+ (6f0d349d).
[...]
I'll add the 3 patches with fixes to v5 of this patchset.
Thanks for investigating. You can fix those three places in your code
OK, will do.
but I was rather looking for a way to check such casting in the future for newly added code. While for the khwasan we can assume it's a debug option, the tagged user pointers are ABI and we need to keep it stable.
We could we actually add some macros for explicit conversion between __user ptr and long and silence the warning there (I guess this would work better for sparse). We can then detect new ptr to long casts as they appear. I just hope that's not too intrusive.
(I haven't tried the sparse patch yet, hopefully sometime this week)
Haven't look at that sparse patch yet myself, but sounds doable. Should these macros go into this patchset or should they go separately?
Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it?
Try using a more up-to-date version of sparse. Odds are you are using an old one, there is a newer version in a different branch on kernel.org somewhere...
greg k-h
Quoting Linus in [1]:
Honestly, I'd like to just encourage people to get the sparse update from Luc Van Oostenryck instead.
For a while there it looked like Chris Li would just pull from Luc, and we'd have timely releases, but that really doesn't seem to have ended up happening after all. So right now it's probably just best to get Luc's tree instead from
https://github.com/lucvoo/sparse-dev
which also ends up fixing a lot of other issues.
[1] https://lore.kernel.org/lkml/CA+55aFzYEnZR2GZLR-DwpONjMNYGYoDy+6AWLCVNayWiaZ... -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
linux-kselftest-mirror@lists.linaro.org
-
Andrey Konovalov -
Catalin Marinas -
David Laight -
Greg Kroah-Hartman -
Luc Van Oostenryck -
Matthew Wilcox -
Ramana Radhakrishnan