arm64 access to userspace addresses in bpf and kprobes is broken, because kernelspace address accessors are always used, and won't work for userspace.
The fix in upstream relies on new kernel BPF API which does not exist in v5.4. The patches here deviate from their upstream sources.
I am not 100% clear on the best way to post a patch series to stable, that's not a direct cherry-pick from upstream. Please let me know if corrections are needed.
Thank you!
Tsahi Zidenberg (2): bpf: fix userspace access for bpf_probe_read{, str}() tracing/kprobes: handle userspace access on unified probes.
arch/arm/Kconfig | 1 + arch/arm64/Kconfig | 1 + arch/powerpc/Kconfig | 1 + arch/x86/Kconfig | 1 + init/Kconfig | 3 +++ kernel/trace/bpf_trace.c | 18 ++++++++++++++++++ kernel/trace/trace_kprobe.c | 15 +++++++++++++++ 7 files changed, 40 insertions(+)
commit 8d92db5c04d10381f4db70ed99b1b576f5db18a7 upstream.
This is an adaptation of parts from above commit to kernel 5.4.
bpf_probe_read{,str}() BPF helpers are broken on arm64, where user addresses cannot be accessed with normal kernelspace access.
Upstream solution got into v5.8 and cannot directly be cherry picked. We implement the same mechanism in kernel 5.4.
Detection is only enabled for machines with non-overlapping address spaces using CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE from commits: commit 0ebeea8ca8a4 ("bpf: Restrict bpf_probe_read{, str}() only to archs where they work") commit d195b1d1d119 ("powerpc/bpf: Enable bpf_probe_read{, str}() on powerpc again")
To generally fix the issue, upstream includes new BPF helpers: bpf_probe_read_{user,kernel}_str(). For details on them, see commit 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers")
Cc: stable@vger.kernel.org # 5.4.x Signed-off-by: Tsahi Zidenberg tsahee@amazon.com --- arch/arm/Kconfig | 1 + arch/arm64/Kconfig | 1 + arch/powerpc/Kconfig | 1 + arch/x86/Kconfig | 1 + init/Kconfig | 3 +++ kernel/trace/bpf_trace.c | 18 ++++++++++++++++++ 6 files changed, 25 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 9aa88715f196..70f4057fb5b0 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -14,6 +14,7 @@ config ARM select ARCH_HAS_KEEPINITRD select ARCH_HAS_KCOV select ARCH_HAS_MEMBARRIER_SYNC_CORE + select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_SPECIAL if ARM_LPAE select ARCH_HAS_PHYS_TO_DMA select ARCH_HAS_SETUP_DMA_OPS diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 9c8ea5939865..a8c49916ab8c 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -22,6 +22,7 @@ config ARM64 select ARCH_HAS_KCOV select ARCH_HAS_KEEPINITRD select ARCH_HAS_MEMBARRIER_SYNC_CORE + select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_DEVMAP select ARCH_HAS_PTE_SPECIAL select ARCH_HAS_SETUP_DMA_OPS diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index c4cbb65e742f..c50bfab7930b 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -129,6 +129,7 @@ config PPC select ARCH_HAS_MMIOWB if PPC64 select ARCH_HAS_PHYS_TO_DMA select ARCH_HAS_PMEM_API + select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_DEVMAP if PPC_BOOK3S_64 select ARCH_HAS_PTE_SPECIAL select ARCH_HAS_MEMBARRIER_CALLBACKS diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 8ef85139553f..b9f666db10c1 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -70,6 +70,7 @@ config X86 select ARCH_HAS_KCOV if X86_64 select ARCH_HAS_MEM_ENCRYPT select ARCH_HAS_MEMBARRIER_SYNC_CORE + select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PMEM_API if X86_64 select ARCH_HAS_PTE_DEVMAP if X86_64 select ARCH_HAS_PTE_SPECIAL diff --git a/init/Kconfig b/init/Kconfig index 96fc45d1b686..1781810d1501 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2210,6 +2210,9 @@ config ASN1 source "kernel/Kconfig.locks" +config ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + bool + config ARCH_HAS_SYNC_CORE_BEFORE_USERMODE bool diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 74c1db7178cf..ef534ad3f94d 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -142,6 +142,15 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr) { int ret; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)unsafe_ptr < TASK_SIZE) { + ret = probe_user_read(dst, unsafe_ptr, size); + if (unlikely(ret < 0)) + goto out; + return ret; + } +#endif + ret = security_locked_down(LOCKDOWN_BPF_READ); if (ret < 0) goto out; @@ -588,6 +597,15 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size, { int ret; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)unsafe_ptr < TASK_SIZE) { + ret = strncpy_from_unsafe_user(dst, (__force void __user *)unsafe_ptr, size); + if (unlikely(ret < 0)) + goto out; + return ret; + } +#endif + ret = security_locked_down(LOCKDOWN_BPF_READ); if (ret < 0) goto out;
On Mon, Mar 29, 2021 at 01:58:21PM +0300, Zidenberg, Tsahi wrote:
commit 8d92db5c04d10381f4db70ed99b1b576f5db18a7 upstream.
This is an adaptation of parts from above commit to kernel 5.4.
This is very different from the upstream commit, let's not annotate it as that commit.
bpf_probe_read{,str}() BPF helpers are broken on arm64, where user addresses cannot be accessed with normal kernelspace access.
Upstream solution got into v5.8 and cannot directly be cherry picked. We implement the same mechanism in kernel 5.4.
Detection is only enabled for machines with non-overlapping address spaces using CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE from commits: commit 0ebeea8ca8a4 ("bpf: Restrict bpf_probe_read{, str}() only to archs where they work") commit d195b1d1d119 ("powerpc/bpf: Enable bpf_probe_read{, str}() on powerpc again")
To generally fix the issue, upstream includes new BPF helpers: bpf_probe_read_{user,kernel}_str(). For details on them, see commit 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers")
What stops us from taking that API back to 5.4? I took a look at the dependencies and they don't look too scary.
Can we try that route instead? We really don't want to diverge from upstream that much.
On 30/03/2021 20:21, Sasha Levin wrote:
commit 8d92db5c04d10381f4db70ed99b1b576f5db18a7 upstream.
This is an adaptation of parts from above commit to kernel 5.4.
This is very different from the upstream commit, let's not annotate it as that commit.
No problem.
To generally fix the issue, upstream includes new BPF helpers: bpf_probe_read_{user,kernel}_str(). For details on them, see commit 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers")
What stops us from taking that API back to 5.4? I took a look at the dependencies and they don't look too scary.
Can we try that route instead? We really don't want to diverge from upstream that much.
probe_read_{user,kernel}* functions themselves seem simple enough. Importing them in a forward-compliant way to 5.4 would require either adding an unused entry in bpf.h's __BPF_FUNC_MAPPER or also pulling skb_output bpf helper functions into 5.4. To me, it seems like too much of a UAPI change to go into a stable release.
Another option would be to import more code to make it somewhat closer to upstream implementation without changing UAPI. As in v5.8, I could internally map these helpers to probe_read_compat* functions, which will in turn call probe_read_{user,kernel}*_common functions. Func names might seem weird out of context, but it will be closer. Implementation will still defer, e.g. to avoid warnings on platforms without ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
Does that sound like a reasonable solution?
-- Thanks, Tsahi
On Wed, Mar 31, 2021 at 09:37:28PM +0300, Zidenberg, Tsahi wrote:
On 30/03/2021 20:21, Sasha Levin wrote:
commit 8d92db5c04d10381f4db70ed99b1b576f5db18a7 upstream.
This is an adaptation of parts from above commit to kernel 5.4.
This is very different from the upstream commit, let's not annotate it as that commit.
No problem.
To generally fix the issue, upstream includes new BPF helpers: bpf_probe_read_{user,kernel}_str(). For details on them, see commit 6ae08ae3dea2 ("bpf: Add probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers")
What stops us from taking that API back to 5.4? I took a look at the dependencies and they don't look too scary.
Can we try that route instead? We really don't want to diverge from upstream that much.
probe_read_{user,kernel}* functions themselves seem simple enough. Importing them in a forward-compliant way to 5.4 would require either adding an unused entry in bpf.h's __BPF_FUNC_MAPPER or also pulling skb_output bpf helper functions into 5.4. To me, it seems like too much of a UAPI change to go into a stable release.
Why is anything changing in the user api here? The user api should not be changing in incompatible ways, otherwise you would not be able to upgrade to new kernels without breaking things.
Another option would be to import more code to make it somewhat closer to upstream implementation without changing UAPI. As in v5.8, I could internally map these helpers to probe_read_compat* functions, which will in turn call probe_read_{user,kernel}*_common functions. Func names might seem weird out of context, but it will be closer. Implementation will still defer, e.g. to avoid warnings on platforms without ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
Does that sound like a reasonable solution?
Again that would make things different from Linus's tree, which is what we want to avoid if at all possible.
What commits in 5.8 are you talking about here, if the changes are there that work here in 5.4, that's fine.
thanks,
greg k-h
On 03/04/2021 12:56, Greg KH wrote:
On Wed, Mar 31, 2021 at 09:37:28PM +0300, Zidenberg, Tsahi wrote:
On 30/03/2021 20:21, Sasha Levin wrote:
What stops us from taking that API back to 5.4? I took a look at the dependencies and they don't look too scary.
Can we try that route instead? We really don't want to diverge from upstream that much.
probe_read_{user,kernel}* functions themselves seem simple enough. Importing them in a forward-compliant way to 5.4 would require either adding an unused entry in bpf.h's __BPF_FUNC_MAPPER or also pulling skb_output bpf helper functions into 5.4. To me, it seems like too much of a UAPI change to go into a stable release.
Why is anything changing in the user api here? The user api should not be changing in incompatible ways, otherwise you would not be able to upgrade to new kernels without breaking things.
Another option would be to import more code to make it somewhat closer to upstream implementation without changing UAPI. As in v5.8, I could internally map these helpers to probe_read_compat* functions, which will in turn call probe_read_{user,kernel}*_common functions. Func names might seem weird out of context, but it will be closer. Implementation will still defer, e.g. to avoid warnings on platforms without ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
Does that sound like a reasonable solution?
Again that would make things different from Linus's tree, which is what we want to avoid if at all possible.
What commits in 5.8 are you talking about here, if the changes are there that work here in 5.4, that's fine.
In 5.5 (mostly 6ae08ae3dea2) BPF UAPI was changed, bpf_probe_read was split into compat (original), user and kernel variants. Compat here just calls the kernel variant, which means it's still broken. In 5.8 (8d92db5c04d10), compat was fixed to call user/kernel variants according to address in machines where it makes sense, and disabled on other machines. I am trying to take the fix for machines where it's possible, and leave other machines untouched.
As I understand it, there are 3 options: 1) Implement the same fix as v5.8, while staying with v5.4 code/API. That's what my original patch did. 2) Import the new 5.5 API + 5.8 fix. It's not trivial to get API-compatibility right. Specifically - need to solve skb_output (a7658e1a4164c), another entry in the BPF enum, introduced before the new read variants. 3) Take some internal code from v5.8 without changing the user-facing API. It will still not be cherry-picks and differ from Linus's tree. Result would be somewhat closer to v5.8 code, at the price of having a larger change.
I still like option 1, but I'd be happy to implement any other option you prefer. I could also submit an identical patchset with a better commit message.
Thank you! Tsahi.
On Sun, Apr 04, 2021 at 12:13:46PM +0300, Zidenberg, Tsahi wrote:
On 03/04/2021 12:56, Greg KH wrote:
On Wed, Mar 31, 2021 at 09:37:28PM +0300, Zidenberg, Tsahi wrote:
On 30/03/2021 20:21, Sasha Levin wrote:
What stops us from taking that API back to 5.4? I took a look at the dependencies and they don't look too scary.
Can we try that route instead? We really don't want to diverge from upstream that much.
probe_read_{user,kernel}* functions themselves seem simple enough. Importing them in a forward-compliant way to 5.4 would require either adding an unused entry in bpf.h's __BPF_FUNC_MAPPER or also pulling skb_output bpf helper functions into 5.4. To me, it seems like too much of a UAPI change to go into a stable release.
Why is anything changing in the user api here? The user api should not be changing in incompatible ways, otherwise you would not be able to upgrade to new kernels without breaking things.
Another option would be to import more code to make it somewhat closer to upstream implementation without changing UAPI. As in v5.8, I could internally map these helpers to probe_read_compat* functions, which will in turn call probe_read_{user,kernel}*_common functions. Func names might seem weird out of context, but it will be closer. Implementation will still defer, e.g. to avoid warnings on platforms without ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
Does that sound like a reasonable solution?
Again that would make things different from Linus's tree, which is what we want to avoid if at all possible.
What commits in 5.8 are you talking about here, if the changes are there that work here in 5.4, that's fine.
In 5.5 (mostly 6ae08ae3dea2) BPF UAPI was changed, bpf_probe_read was split into compat (original), user and kernel variants. Compat here just calls the kernel variant, which means it's still broken.
That's not a UAPI change, that does not change the userspace-visable part, right? Did something change?
In 5.8 (8d92db5c04d10), compat was fixed to call user/kernel variants according to address in machines where it makes sense, and disabled on other machines. I am trying to take the fix for machines where it's possible, and leave other machines untouched.
As I understand it, there are 3 options: 1) Implement the same fix as v5.8, while staying with v5.4 code/API. That's what my original patch did. 2) Import the new 5.5 API + 5.8 fix. It's not trivial to get API-compatibility right. Specifically - need to solve skb_output (a7658e1a4164c), another entry in the BPF enum, introduced before the new read variants.
What "API-compatibility" are you trying for here? There is no issues with in-kernel changes of apis.
What commits exactly does this require? It is almost always better to keep the same identical patches that are in newer kernels to be backported than to do something different like you are doing in 1). That way any future changes/fixes can easily also be backported. Otherwise it gets harder and harder over time.
thanks,
greg k-h
On 10/04/2021 14:29, Greg KH wrote:
On Sun, Apr 04, 2021 at 12:13:46PM +0300, Zidenberg, Tsahi wrote:
On 03/04/2021 12:56, Greg KH wrote:
Again that would make things different from Linus's tree, which is what we want to avoid if at all possible.
What commits in 5.8 are you talking about here, if the changes are there that work here in 5.4, that's fine.
In 5.5 (mostly 6ae08ae3dea2) BPF UAPI was changed, bpf_probe_read was split into compat (original), user and kernel variants. Compat here just calls the kernel variant, which means it's still broken.
That's not a UAPI change, that does not change the userspace-visable part, right? Did something change?
In 5.8 (8d92db5c04d10), compat was fixed to call user/kernel variants according to address in machines where it makes sense, and disabled on other machines. I am trying to take the fix for machines where it's possible, and leave other machines untouched.
As I understand it, there are 3 options:
- Implement the same fix as v5.8, while staying with v5.4 code/API. That's what my original patch did.
- Import the new 5.5 API + 5.8 fix. It's not trivial to get API-compatibility right. Specifically - need to solve skb_output (a7658e1a4164c), another entry in the BPF enum, introduced before the new read variants.
What "API-compatibility" are you trying for here? There is no issues with in-kernel changes of apis.
What commits exactly does this require? It is almost always better to keep the same identical patches that are in newer kernels to be backported than to do something different like you are doing in 1). That way any future changes/fixes can easily also be backported. Otherwise it gets harder and harder over time.
This is how I understand it, please correct me if/where I'm wrong:
include/uapi/linux/bpf.h is part of the UAPI. Specifically, bpf_func_id enum is part of the UAPI. This enum matches function I.D to bpf helper, and the indexes are kept constant across kernel versions.
Kernel 5.5 added skb_output helper (irrelevant to our fix) to that enum, and then added probe_read_{user,kernel}* functions on top of that. Taking probe_read_{user,kernel}* from commit 6ae08ae3dea2 itself is changing UAPI. The mainline fix in 5.8 (8d92db5c04d10) depends on that UAPI change.
Appending another function is not a terrrible UAPI change, but to keep these functions at the same index as later kernel versions - we'd also need to either take skb_output or add a replacement.
Thank you! Tsahi.
On Mon, Apr 12, 2021 at 11:01:59PM +0300, Zidenberg, Tsahi wrote:
On 10/04/2021 14:29, Greg KH wrote:
On Sun, Apr 04, 2021 at 12:13:46PM +0300, Zidenberg, Tsahi wrote:
On 03/04/2021 12:56, Greg KH wrote:
Again that would make things different from Linus's tree, which is what we want to avoid if at all possible.
What commits in 5.8 are you talking about here, if the changes are there that work here in 5.4, that's fine.
In 5.5 (mostly 6ae08ae3dea2) BPF UAPI was changed, bpf_probe_read was split into compat (original), user and kernel variants. Compat here just calls the kernel variant, which means it's still broken.
That's not a UAPI change, that does not change the userspace-visable part, right? Did something change?
In 5.8 (8d92db5c04d10), compat was fixed to call user/kernel variants according to address in machines where it makes sense, and disabled on other machines. I am trying to take the fix for machines where it's possible, and leave other machines untouched.
As I understand it, there are 3 options:
- Implement the same fix as v5.8, while staying with v5.4 code/API. That's what my original patch did.
- Import the new 5.5 API + 5.8 fix. It's not trivial to get API-compatibility right. Specifically - need to solve skb_output (a7658e1a4164c), another entry in the BPF enum, introduced before the new read variants.
What "API-compatibility" are you trying for here? There is no issues with in-kernel changes of apis.
What commits exactly does this require? It is almost always better to keep the same identical patches that are in newer kernels to be backported than to do something different like you are doing in 1). That way any future changes/fixes can easily also be backported. Otherwise it gets harder and harder over time.
This is how I understand it, please correct me if/where I'm wrong:
include/uapi/linux/bpf.h is part of the UAPI. Specifically, bpf_func_id enum is part of the UAPI. This enum matches function I.D to bpf helper, and the indexes are kept constant across kernel versions.
Kernel 5.5 added skb_output helper (irrelevant to our fix) to that enum, and then added probe_read_{user,kernel}* functions on top of that. Taking probe_read_{user,kernel}* from commit 6ae08ae3dea2 itself is changing UAPI. The mainline fix in 5.8 (8d92db5c04d10) depends on that UAPI change.
So you are just adding new things, not changing existing things. There's nothing wrong with adding new userspace apis, nothing is "frozen" in that existing userspace would suddenly be broken here, right?
Appending another function is not a terrrible UAPI change, but to keep these functions at the same index as later kernel versions - we'd also need to either take skb_output or add a replacement.
Yes you need to keep the values identical, that's fine, and expected, we do that all the time in stable kernels.
thanks,
greg k-h
commit 9de1fec50b23117f0a19f7609cc837ca72e764a6 upstream.
This is an adaptation of parts from the above commit to kernel 5.4.
Allow Kprobes to access userspace data correctly in architectures with no overlap between kernel and userspace addresses.
Cc: stable@vger.kernel.org # 5.4.x Signed-off-by: Tsahi Zidenberg tsahee@amazon.com --- kernel/trace/trace_kprobe.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 233322c77b76..cbd72a1c9530 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -1043,6 +1043,11 @@ fetch_store_strlen(unsigned long addr) int ret, len = 0; u8 c; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if (addr < TASK_SIZE) + return fetch_store_strlen_user(addr); +#endif + do { ret = probe_kernel_read(&c, (u8 *)addr + len, 1); len++; @@ -1071,6 +1076,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base) void *__dest; long ret; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if (addr < TASK_SIZE) + return fetch_store_string_user(addr, dest, base); +#endif + if (unlikely(!maxlen)) return -ENOMEM; @@ -1114,6 +1124,11 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base) static nokprobe_inline int probe_mem_read(void *dest, void *src, size_t size) { +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)src < TASK_SIZE) + return probe_mem_read_user(dest, src, size); +#endif + return probe_kernel_read(dest, src, size); }
On Mon, Mar 29, 2021 at 01:59:48PM +0300, Zidenberg, Tsahi wrote:
commit 9de1fec50b23117f0a19f7609cc837ca72e764a6 upstream.
This is an adaptation of parts from the above commit to kernel 5.4.
Allow Kprobes to access userspace data correctly in architectures with no overlap between kernel and userspace addresses.
Cc: stable@vger.kernel.org # 5.4.x Signed-off-by: Tsahi Zidenberg tsahee@amazon.com
kernel/trace/trace_kprobe.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 233322c77b76..cbd72a1c9530 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -1043,6 +1043,11 @@ fetch_store_strlen(unsigned long addr) int ret, len = 0; u8 c; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if (addr < TASK_SIZE) + return fetch_store_strlen_user(addr); +#endif
do { ret = probe_kernel_read(&c, (u8 *)addr + len, 1); len++; @@ -1071,6 +1076,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base) void *__dest; long ret; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if (addr < TASK_SIZE) + return fetch_store_string_user(addr, dest, base); +#endif
if (unlikely(!maxlen)) return -ENOMEM; @@ -1114,6 +1124,11 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base) static nokprobe_inline int probe_mem_read(void *dest, void *src, size_t size) { +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)src < TASK_SIZE) + return probe_mem_read_user(dest, src, size); +#endif
return probe_kernel_read(dest, src, size); } -- 2.25.1
What problem is this fixing?
thanks,
greg k-h
On Mon, Mar 29, 2021 at 01:56:53PM +0300, Zidenberg, Tsahi wrote:
arm64 access to userspace addresses in bpf and kprobes is broken, because kernelspace address accessors are always used, and won't work for userspace.
What does not work exactly?
What is broken that is fixed in these changes? I can't seem to understand that as it feels like bpf and kprobes works on 5.4.y unless something broke it?
confused,
greg k-h
On 10/04/2021 14:30, Greg KH wrote:
On Mon, Mar 29, 2021 at 01:56:53PM +0300, Zidenberg, Tsahi wrote:
arm64 access to userspace addresses in bpf and kprobes is broken, because kernelspace address accessors are always used, and won't work for userspace.
What does not work exactly?
What is broken that is fixed in these changes? I can't seem to understand that as it feels like bpf and kprobes works on 5.4.y unless something broke it?
confused,
greg k-h
The original bug that I was working on: command line parameters don't appear when snooping execve using bpf on arm64. This is true using either osquery (with --enable_bpf_events) or bcc (execsnoop-bpfcc). The reason, it seems, is that in arm64 userspace addresses cannot be accessed with kernelspace accessors. This bug is fixed with Patch 1.
Since Patch 1 added ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, I thought it made sense to check what else uses this config. I did not verify kprobes are also broken in the same way, but it seems likely, and the fix is very small. If only Patch 1 is merged - I'll be happy :)
Thank you! Tsahi.
On Mon, Apr 12, 2021 at 10:46:41PM +0300, Zidenberg, Tsahi wrote:
On 10/04/2021 14:30, Greg KH wrote:
On Mon, Mar 29, 2021 at 01:56:53PM +0300, Zidenberg, Tsahi wrote:
arm64 access to userspace addresses in bpf and kprobes is broken, because kernelspace address accessors are always used, and won't work for userspace.
What does not work exactly?
What is broken that is fixed in these changes? I can't seem to understand that as it feels like bpf and kprobes works on 5.4.y unless something broke it?
confused,
greg k-h
The original bug that I was working on: command line parameters don't appear when snooping execve using bpf on arm64.
Has this ever worked? If not, it's not really a regression that needs to be fixed, just use a newer kernel version, right?
This is true using either osquery (with --enable_bpf_events) or bcc (execsnoop-bpfcc). The reason, it seems, is that in arm64 userspace addresses cannot be accessed with kernelspace accessors. This bug is fixed with Patch 1.
Since Patch 1 added ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE, I thought it made sense to check what else uses this config. I did not verify kprobes are also broken in the same way, but it seems likely, and the fix is very small. If only Patch 1 is merged - I'll be happy :)
But your "patch 1" is no where near what that commit is upstream so now you have divered from what is there and created something new. And we don't like that for obvious reasons (no testing, maintaining over time, etc.)
So again, if you want to do this type of thing, why not just use a newer kernel?
thanks,
greg k-h
On 13/04/2021 10:27, Greg KH wrote:
On Mon, Apr 12, 2021 at 10:46:41PM +0300, Zidenberg, Tsahi wrote:
The original bug that I was working on: command line parameters don't appear when snooping execve using bpf on arm64.
Has this ever worked? If not, it's not really a regression that needs to be fixed, just use a newer kernel version, right?
It's not so much a regression between old and new kernels, as it is a regression when changing architectures. The same API works on x86, but not on arm64 (in x86 - you can access userspace with a kernelspace access). Multiple Linux distributions support both x86 and arm64, and some of these choose to keep with 5.4 LTS Linux kernel. I think these distributions should expect the same experience across architectures.
But your "patch 1" is no where near what that commit is upstream so now you have divered from what is there and created something new. And we don't like that for obvious reasons (no testing, maintaining over time, etc.)
I have created an alternative patch series that stays very close to upstream. It brings in much more code, and adds some APIs as we discussed. Will send it right away for your consideration.
Thank you! Tsahi.
On Wed, Apr 21, 2021 at 04:04:12PM +0300, Zidenberg, Tsahi wrote:
On 13/04/2021 10:27, Greg KH wrote:
On Mon, Apr 12, 2021 at 10:46:41PM +0300, Zidenberg, Tsahi wrote:
The original bug that I was working on: command line parameters don't appear when snooping execve using bpf on arm64.
Has this ever worked? If not, it's not really a regression that needs to be fixed, just use a newer kernel version, right?
It's not so much a regression between old and new kernels, as it is a regression when changing architectures. The same API works on x86, but not on arm64 (in x86 - you can access userspace with a kernelspace access).
What API are you talking about here?
Different CPU architectures support different things. Some do not support ebpf at all, is that a regression? No.
Multiple Linux distributions support both x86 and arm64, and some of these choose to keep with 5.4 LTS Linux kernel. I think these distributions should expect the same experience across architectures.
Since when has that ever been a requirement of Linux?
That is not a requirement of the stable kernel trees either, please see our very simple set of rules.
still confused as to what you are trying to do here,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Greg KH -
Sasha Levin -
Zidenberg, Tsahi