| Message ID | 56be4b97-8283-cf09-4dac-46d602cae97c@amazon.com |
|---|---|
| Headers | show |
| Series | fix userspace access on arm64 for linux 5.4 | expand |
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
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 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 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.
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 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
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
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(+)