Messages in this thread | | | Date | Mon, 6 May 2019 16:58:07 -0400 | From | Joel Fernandes <> | Subject | Re: [PATCH RFC] bpf: Add support for reading user pointers |
| |
On Mon, May 06, 2019 at 07:35:06PM +0100, Will Deacon wrote: > Hi Joel, > > On Sun, May 05, 2019 at 02:03:13PM -0400, Joel Fernandes wrote: > > +Mark, Will since discussion is about arm64 arch code. > > > > The difference between observing the bug and everything just working seems to > > be the set_fs(USER_DS) as done by Masami's patch that this patch is based on. > > The following diff shows 'ret' as 255 when set_fs(KERN_DS) is used, and then > > after we retry with set_fs(USER_DS), the read succeeds. > > > > diff --git a/mm/maccess.c b/mm/maccess.c > > index 78f9274dd49d..d3e01a33c712 100644 > > --- a/mm/maccess.c > > +++ b/mm/maccess.c > > @@ -32,9 +32,20 @@ long __probe_kernel_read(void *dst, const void *src, size_t size) > > pagefault_disable(); > > ret = __copy_from_user_inatomic(dst, > > (__force const void __user *)src, size); > > + trace_printk("KERNEL_DS: __copy_from_user_inatomic: ret=%d\n", ret); > > pagefault_enable(); > > set_fs(old_fs); > > > > + if (ret) { > > + set_fs(USER_DS); > > + pagefault_disable(); > > + ret = __copy_from_user_inatomic(dst, > > + (__force const void __user *)src, size); > > + trace_printk("RETRY WITH USER_DS: __copy_from_user_inatomic: ret=%d\n", ret); > > + pagefault_enable(); > > + set_fs(old_fs); > > + } > > + > > return ret ? -EFAULT : 0; > > } > > EXPORT_SYMBOL_GPL(probe_kernel_read); > > > > In initially thought this was because of the addr_limit pointer masking done > > by this patch from Mark Rutland "arm64: Use pointer masking to limit uaccess > > speculation" > > > > However removing this masking still makes it fail with KERNEL_DS. > > > > Fwiw, I am still curious which other paths in arm64 check the addr_limit > > which might make the __copy_from_user_inatomic fail if the set_fs is not > > setup correctly. > > > > Either way, I will resubmit the patch with the commit message fixed correctly > > as we agreed and also address Alexei's comments. > > I'm coming at this with no background, so it's tricky to understand exactly > what's going on here. Some questions:
No problem, I added you out of the blue so it is quite understandable :)
> * Are you seeing a failure with mainline and/or an official stable kernel?
This issue is noticed on the Pixel3 kernel (4.9 based): git clone https://android.googlesource.com/kernel/msm (branch: android-msm-crosshatch-4.9-q-preview-1)
> * What is the failing CPU? (so we can figure out which architectural > extensions are implemented) From cpuinfo: AArch64 Processor rev 12 (aarch64) (Qualcomm SDM845 SoC). It is a Pixel 3 phone.
> * Do you have a .config anywhere? Particular, how are ARM64_PAN, > ARM64_TTBR0_PAN and ARM64_UAO set?
CONFIG_ARM64_SW_TTBR0_PAN is not set CONFIG_ARM64_PAN=y CONFIG_ARM64_UAO=y
I wanted to say I enabled SW_TTBR0_PAN config and also got the same result.
> * Is the address being accessed a user or a kernel address?
User. It is the second argument of do_sys_open() kernel function. kprobe gives bpf the pointer which the bpf program dereferences with probe_kernel_read.
> If you're trying to dereference a pointer to userspace using > probe_kernel_read(), that clearly isn't going to work.
Ok. Thanks for confirming as well. The existing code has this bug and these patches fix it.
- Joel
| |