[PATCH 0/2] fix userspace access on arm64 for linux 5.4

[PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Zidenberg, Tsahi]

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(+)

-- 
2.25.1

[PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Zidenberg, Tsahi]

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;
-- 
2.25.1

[PATCH 2/2] tracing/kprobes: handle userspace access on unified probes

[Zidenberg, Tsahi]

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

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Sasha Levin]

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.

-- 
Thanks,
Sasha

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Zidenberg, Tsahi]

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

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Greg KH]

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

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Zidenberg, Tsahi]

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.

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Greg KH]

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

Re: [PATCH 2/2] tracing/kprobes: handle userspace access on unified probes

[Greg KH]

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

Re: [PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Greg KH]

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

Re: [PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Zidenberg, Tsahi]

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.

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Zidenberg, Tsahi]

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

Re: [PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Greg KH]

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

Re: [PATCH 1/2] bpf: fix userspace access for bpf_probe_read{, str}()

[Greg KH]

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

Re: [PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Zidenberg, Tsahi]

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.

Re: [PATCH 0/2] fix userspace access on arm64 for linux 5.4

[Greg KH]

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