| Message ID | 20210420165001.3790670-1-Liam.Howlett@Oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] arm64: armv8_deprecated: Fix swp_handler() signal generation | expand |
* kernel test robot <lkp@intel.com> [210420 17:22]: > Hi Liam, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on arm64/for-next/core] > [also build test WARNING on arm-perf/for-next/perf xlnx/master arm/for-next soc/for-next kvmarm/next linus/master v5.12-rc8 next-20210420] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Liam-Howlett/arm64-armv8_deprecated-Fix-swp_handler-signal-generation/20210421-005252 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core > config: arm64-allyesconfig (attached as .config) > compiler: aarch64-linux-gcc (GCC) 9.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/96a011695861072d32851ba3a104b19106955869 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Liam-Howlett/arm64-armv8_deprecated-Fix-swp_handler-signal-generation/20210421-005252 > git checkout 96a011695861072d32851ba3a104b19106955869 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=arm64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > arch/arm64/kernel/armv8_deprecated.c: In function 'swp_handler': > >> arch/arm64/kernel/armv8_deprecated.c:418:11: warning: suggest parentheses around assignment used as truth value [-Wparentheses] > 418 | else if (res = -ENXIO) /* Unaligned pointer */ > | ^~~ > Ah, I hadn't had the correct config enabled for this code to be compiled. I will wait for any other comments then I'll send out a v2 with this fix. Thanks, Liam
On Tue, Apr 20, 2021 at 04:50:13PM +0000, Liam Howlett wrote: > find_vma() will continue to search upwards until the end of the virtual > memory space. This means the si_code would almost never be set to > SEGV_MAPERR even when the address falls outside of any VMA. The result > is that the si_code is not reliable as it may or may not be set to the > correct result, depending on where the address falls in the address > space. > > Using find_vma_intersection() allows for what is intended by only > returning a VMA if it falls within the range provided, in this case a > window of 1. > > Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm) > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > --- > arch/arm64/kernel/traps.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index a05d34f0e82a..a44007904a64 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -383,9 +383,10 @@ void force_signal_inject(int signal, int code, unsigned long address, unsigned i > void arm64_notify_segfault(unsigned long addr) > { > int code; > + unsigned long ut_addr = untagged_addr(addr); > > mmap_read_lock(current->mm); > - if (find_vma(current->mm, untagged_addr(addr)) == NULL) > + if (find_vma_intersection(current->mm, ut_addr, ut_addr + 1) == NULL) > code = SEGV_MAPERR; > else > code = SEGV_ACCERR; I'm not seeing how this addresses VM_GROWSDOWN as Catalin mentioned before. Will
On Tue, Apr 20, 2021 at 04:50:12PM +0000, Liam Howlett wrote: > arm64_notify_segfault() was written to decide on the si_code from the > assembly emulation of the swp_handler(), but was also used for the > signal generation from failed access_ok() and unaligned instructions. > > When access_ok() fails, there is no need to search for the offending > address in the VMA space. Instead, simply set the error to SIGSEGV with > si_code SEGV_ACCERR. > > Change the return code from emulate_swpX() when there is an unaligned > pointer so the caller can differentiate from the EFAULT. It is > unnecessary to search the VMAs in the case of an unaligned pointer. > This change uses SIGSEGV and SEGV_ACCERR instead of SIGBUS to keep with > what was returned before. > > Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm) > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > --- > arch/arm64/kernel/armv8_deprecated.c | 20 +++++++++++++------- Can you give an example of something that is fixed by this, please? At first glance, it doesn't look like it changes the user-visible behaviour. We should also be compatible with arch/arm/ here (see set_segfault()). Will
* Will Deacon <will@kernel.org> [210422 08:53]: > On Tue, Apr 20, 2021 at 04:50:13PM +0000, Liam Howlett wrote: > > find_vma() will continue to search upwards until the end of the virtual > > memory space. This means the si_code would almost never be set to > > SEGV_MAPERR even when the address falls outside of any VMA. The result > > is that the si_code is not reliable as it may or may not be set to the > > correct result, depending on where the address falls in the address > > space. > > > > Using find_vma_intersection() allows for what is intended by only > > returning a VMA if it falls within the range provided, in this case a > > window of 1. > > > > Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm) > > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > > --- > > arch/arm64/kernel/traps.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > index a05d34f0e82a..a44007904a64 100644 > > --- a/arch/arm64/kernel/traps.c > > +++ b/arch/arm64/kernel/traps.c > > @@ -383,9 +383,10 @@ void force_signal_inject(int signal, int code, unsigned long address, unsigned i > > void arm64_notify_segfault(unsigned long addr) > > { > > int code; > > + unsigned long ut_addr = untagged_addr(addr); > > > > mmap_read_lock(current->mm); > > - if (find_vma(current->mm, untagged_addr(addr)) == NULL) > > + if (find_vma_intersection(current->mm, ut_addr, ut_addr + 1) == NULL) > > code = SEGV_MAPERR; > > else > > code = SEGV_ACCERR; > > I'm not seeing how this addresses VM_GROWSDOWN as Catalin mentioned before. Sorry for not including the link before, but for context, I've added the URL for the lore conversation from before at the end of my message [1]. I thought this was resolved by the fact that the stack expansion would have already taken care of the VM_GROWSDOWN code path? Did I misunderstand what was said? I've modified the other paths to this function to avoid causing issues elsewhere and to hopefully do the necessary cleanup that Catalin said needed to be tidied. Link: https://lore.kernel.org/lkml/20210413180030.GA31164@arm.com/ Thanks, Liam
* Will Deacon <will@kernel.org> [210422 09:01]: > On Tue, Apr 20, 2021 at 04:50:12PM +0000, Liam Howlett wrote: > > arm64_notify_segfault() was written to decide on the si_code from the > > assembly emulation of the swp_handler(), but was also used for the > > signal generation from failed access_ok() and unaligned instructions. > > > > When access_ok() fails, there is no need to search for the offending > > address in the VMA space. Instead, simply set the error to SIGSEGV with > > si_code SEGV_ACCERR. > > > > Change the return code from emulate_swpX() when there is an unaligned > > pointer so the caller can differentiate from the EFAULT. It is > > unnecessary to search the VMAs in the case of an unaligned pointer. > > This change uses SIGSEGV and SEGV_ACCERR instead of SIGBUS to keep with > > what was returned before. > > > > Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm) > > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> > > --- > > arch/arm64/kernel/armv8_deprecated.c | 20 +++++++++++++------- > > Can you give an example of something that is fixed by this, please? At first > glance, it doesn't look like it changes the user-visible behaviour. In short, when !access_ok(), don't return SEGV_MAPERR. access_ok() is defined as __range_ok() which checks if the address is a userspace address. If the access is not okay, then the return should be SEGV_ACCERR. However, if the address is above any known VMA, then the return will be SEGV_MAPERR. Isn't this a bug? Right now this particular bug is masked almost always by the fact that find_vma() will return SIGV_ACCERR unless it's abvove any known VMA, but patch 3 in this series will alter the behaviour and thus, I wanted to fix the bug here before fixing that bug. > > We should also be compatible with arch/arm/ here (see set_segfault()). Yes, the same error exists there it seems. If my solution is acceptable, I can expand it to include the same change there. Thanks, Liam
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c index 0e86e8b9cedd..f424082b3455 100644 --- a/arch/arm64/kernel/armv8_deprecated.c +++ b/arch/arm64/kernel/armv8_deprecated.c @@ -324,7 +324,7 @@ static int emulate_swpX(unsigned int address, unsigned int *data, if ((type != TYPE_SWPB) && (address & 0x3)) { /* SWP to unaligned address not permitted */ pr_debug("SWP instruction on unaligned pointer!\n"); - return -EFAULT; + return -ENXIO; } while (1) { @@ -406,15 +406,17 @@ static int swp_handler(struct pt_regs *regs, u32 instr) user_ptr = (const void __user *)(unsigned long)(address & ~3); if (!access_ok(user_ptr, 4)) { pr_debug("SWP{B} emulation: access to 0x%08x not allowed!\n", - address); - goto fault; + address); + goto e_access; } res = emulate_swpX(address, &data, type); - if (res == -EFAULT) - goto fault; - else if (res == 0) + if (!res) regs->user_regs.regs[destreg] = data; + else if (res == -EFAULT) + goto e_fault; + else if (res = -ENXIO) /* Unaligned pointer */ + goto e_align; ret: if (type == TYPE_SWPB) @@ -428,10 +430,14 @@ static int swp_handler(struct pt_regs *regs, u32 instr) arm64_skip_faulting_instruction(regs, 4); return 0; -fault: +e_fault: pr_debug("SWP{B} emulation: access caused memory abort!\n"); arm64_notify_segfault(address); + return 0; +e_align: +e_access: + force_signal_inject(SIGSEGV, SEGV_ACCERR, address, 0); return 0; }
arm64_notify_segfault() was written to decide on the si_code from the assembly emulation of the swp_handler(), but was also used for the signal generation from failed access_ok() and unaligned instructions. When access_ok() fails, there is no need to search for the offending address in the VMA space. Instead, simply set the error to SIGSEGV with si_code SEGV_ACCERR. Change the return code from emulate_swpX() when there is an unaligned pointer so the caller can differentiate from the EFAULT. It is unnecessary to search the VMAs in the case of an unaligned pointer. This change uses SIGSEGV and SEGV_ACCERR instead of SIGBUS to keep with what was returned before. Fixes: bd35a4adc413 (arm64: Port SWP/SWPB emulation support from arm) Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com> --- arch/arm64/kernel/armv8_deprecated.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)