arm64: untag user pointers passed to the kernel

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

Quoted 12 lines · Andrey Konovalov

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.

Quoted 3 lines

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.

Hi Andrey,

Quoted 13 lines · Andrey Konovalov

Quoted 12 lines · Andrey Konovalov

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.

Quoted 8 lines

Quoted 3 lines

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?

Quoted 32 lines · Catalin Marinas

Hi Andrey,

Quoted 13 lines · Andrey Konovalov

Quoted 12 lines · Andrey Konovalov

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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!

Quoted 38 lines · Andrey Konovalov

Quoted 35 lines · Catalin Marinas

Quoted 13 lines · Andrey Konovalov

Quoted 12 lines · Andrey Konovalov

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.

Quoted 8 lines

Quoted 3 lines

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?

Quoted 1 line

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.

Quoted 4 lines · Catalin Marinas

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?

Quoted 6 lines · Luc Van Oostenryck

Quoted 4 lines · Catalin Marinas

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.

Quoted 2 lines

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.

Quoted 10 lines · Catalin Marinas

Quoted 8 lines · Luc Van Oostenryck

Quoted 4 lines · Catalin Marinas

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?

Quoted 5 lines

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.

Quoted 5 lines

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).

Quoted 17 lines · Luc Van Oostenryck

Quoted 15 lines · Catalin Marinas

Quoted 8 lines · Luc Van Oostenryck

Quoted 4 lines · Catalin Marinas

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.

Quoted 1 line

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).

Sent: 28 June 2018 15:49

Quoted 8 lines

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.

Quoted 5 lines · Catalin Marinas

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.

Quoted 10 lines · Andrey Konovalov

Quoted 5 lines · Catalin Marinas

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

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
???

Quoted 10 lines · Andrey Konovalov

Quoted 5 lines · Catalin Marinas

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?

Quoted 11 lines · Andrey Konovalov

Quoted 9 lines · Andrey Konovalov

Quoted 5 lines · Catalin Marinas

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.

Quoted 7 lines

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.

Quoted 5 lines · Andrey Konovalov

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).

[...]

Quoted 1 line

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)

Quoted 10 lines · Catalin Marinas

Quoted 5 lines · Andrey Konovalov

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).

[...]

Quoted 1 line

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.

Quoted 9 lines

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.

Quoted 1 line

(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?

Quoted 23 lines · Andrey Konovalov

Quoted 10 lines · Catalin Marinas

Quoted 5 lines · Andrey Konovalov

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).

[...]

Quoted 1 line

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.

Quoted 9 lines

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.

Quoted 5 lines

Quoted 1 line

(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?

Quoted 2 lines · Andrey Konovalov

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...

Quoted 4 lines · Greg Kroah-Hartman

Quoted 2 lines · Andrey Konovalov

Started looking at this. When I run sparse with default checks enabled (make C=1) I get countless warnings. Does anybody actually use it?

Quoted 3 lines

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:

Quoted 30 lines · Andrey Konovalov

Quoted 28 lines · Andrey Konovalov

Quoted 10 lines · Catalin Marinas

Quoted 5 lines · Andrey Konovalov

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).

[...]

Quoted 1 line

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.

Quoted 10 lines

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?

Quoted 2 lines

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