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
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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;
+}
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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;
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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();
--
2.18.0.rc1.244.gcf134e6275-goog
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 <[email protected]>
---
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)) {
--
2.18.0.rc1.244.gcf134e6275-goog
On Wed, Jun 20, 2018 at 5:24 PM, Andrey Konovalov <[email protected]> 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!
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 <[email protected]> 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.
--
Catalin
On 27/06/2018 16:05, Andrey Konovalov wrote:
> On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
> <[email protected]> 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 <[email protected]> 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!
>
On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
<[email protected]> 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 <[email protected]> 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!
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
> > <[email protected]> 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 <[email protected]> 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.
--
Catalin
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
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.
--
Catalin
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
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
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.
--
Catalin
On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov <[email protected]> wrote:
> On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
> <[email protected]> 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
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/[email protected]/
Signed-off-by: Luc Van Oostenryck <[email protected]>
---
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
+ */
--
2.18.0
On Fri, Jun 29, 2018 at 5:19 PM, Andrey Konovalov <[email protected]> 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.
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
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 <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov <[email protected]> wrote:
>> On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
>> <[email protected]> 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
On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov <[email protected]> wrote:
>> On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
>> <[email protected]> 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
On Mon, Jul 16, 2018 at 1:25 PM, Andrey Konovalov <[email protected]> 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
On Mon, Jul 16, 2018 at 01:25:59PM +0200, Andrey Konovalov wrote:
> On Thu, Jun 28, 2018 at 9:30 PM, Andrey Konovalov <[email protected]> wrote:
> > On Wed, Jun 27, 2018 at 5:05 PM, Andrey Konovalov <[email protected]> wrote:
> >> On Tue, Jun 26, 2018 at 7:29 PM, Catalin Marinas
> >> <[email protected]> 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)
--
Catalin
On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas <[email protected]> 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 <[email protected]> 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?
On Thu, Aug 2, 2018 at 5:00 PM, Andrey Konovalov <[email protected]> wrote:
> On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas <[email protected]> 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 <[email protected]> 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?
On Fri, Aug 03, 2018 at 04:59:18PM +0200, Andrey Konovalov wrote:
> On Thu, Aug 2, 2018 at 5:00 PM, Andrey Konovalov <[email protected]> wrote:
> > On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas <[email protected]> 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 <[email protected]> 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
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.
On Fri, Aug 3, 2018 at 6:43 PM, Matthew Wilcox <[email protected]> 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
On Fri, Aug 3, 2018 at 5:09 PM, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]> wrote:
>> > On Wed, Aug 1, 2018 at 7:42 PM, Catalin Marinas <[email protected]> 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 <[email protected]> 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+6AWLCVNayWiaZuqoA@mail.gmail.com/T/#u