[1/3] arm64: armv8_deprecated: Fix swp_handler() signal generation
diff mbox series

Message ID 20210420165001.3790670-1-Liam.Howlett@Oracle.com
State New, archived
Headers show
Series
  • [1/3] arm64: armv8_deprecated: Fix swp_handler() signal generation
Related show

Commit Message

Liam Howlett April 20, 2021, 4:50 p.m. UTC
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(-)

Comments

kernel test robot April 20, 2021, 9:22 p.m. UTC | #1
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 */
         |           ^~~


vim +418 arch/arm64/kernel/armv8_deprecated.c

   365	
   366	/*
   367	 * swp_handler logs the id of calling process, dissects the instruction, sanity
   368	 * checks the memory location, calls emulate_swpX for the actual operation and
   369	 * deals with fixup/error handling before returning
   370	 */
   371	static int swp_handler(struct pt_regs *regs, u32 instr)
   372	{
   373		u32 destreg, data, type, address = 0;
   374		const void __user *user_ptr;
   375		int rn, rt2, res = 0;
   376	
   377		perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->pc);
   378	
   379		type = instr & TYPE_SWPB;
   380	
   381		switch (aarch32_check_condition(instr, regs->pstate)) {
   382		case ARM_OPCODE_CONDTEST_PASS:
   383			break;
   384		case ARM_OPCODE_CONDTEST_FAIL:
   385			/* Condition failed - return to next instruction */
   386			goto ret;
   387		case ARM_OPCODE_CONDTEST_UNCOND:
   388			/* If unconditional encoding - not a SWP, undef */
   389			return -EFAULT;
   390		default:
   391			return -EINVAL;
   392		}
   393	
   394		rn = aarch32_insn_extract_reg_num(instr, A32_RN_OFFSET);
   395		rt2 = aarch32_insn_extract_reg_num(instr, A32_RT2_OFFSET);
   396	
   397		address = (u32)regs->user_regs.regs[rn];
   398		data	= (u32)regs->user_regs.regs[rt2];
   399		destreg = aarch32_insn_extract_reg_num(instr, A32_RT_OFFSET);
   400	
   401		pr_debug("addr in r%d->0x%08x, dest is r%d, source in r%d->0x%08x)\n",
   402			rn, address, destreg,
   403			aarch32_insn_extract_reg_num(instr, A32_RT2_OFFSET), data);
   404	
   405		/* Check access in reasonable access range for both SWP and SWPB */
   406		user_ptr = (const void __user *)(unsigned long)(address & ~3);
   407		if (!access_ok(user_ptr, 4)) {
   408			pr_debug("SWP{B} emulation: access to 0x%08x not allowed!\n",
   409				 address);
   410			goto e_access;
   411		}
   412	
   413		res = emulate_swpX(address, &data, type);
   414		if (!res)
   415			regs->user_regs.regs[destreg] = data;
   416		else if (res == -EFAULT)
   417			goto e_fault;
 > 418		else if (res = -ENXIO) /* Unaligned pointer */
   419			goto e_align;
   420	
   421	ret:
   422		if (type == TYPE_SWPB)
   423			trace_instruction_emulation("swpb", regs->pc);
   424		else
   425			trace_instruction_emulation("swp", regs->pc);
   426	
   427		pr_warn_ratelimited("\"%s\" (%ld) uses obsolete SWP{B} instruction at 0x%llx\n",
   428				current->comm, (unsigned long)current->pid, regs->pc);
   429	
   430		arm64_skip_faulting_instruction(regs, 4);
   431		return 0;
   432	
   433	e_fault:
   434		pr_debug("SWP{B} emulation: access caused memory abort!\n");
   435		arm64_notify_segfault(address);
   436		return 0;
   437	
   438	e_align:
   439	e_access:
   440		force_signal_inject(SIGSEGV, SEGV_ACCERR, address, 0);
   441		return 0;
   442	}
   443	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Liam Howlett April 21, 2021, 3:47 p.m. UTC | #2
* 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
Will Deacon April 22, 2021, 12:59 p.m. UTC | #3
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
Liam Howlett April 23, 2021, 1 a.m. UTC | #4
* 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

Patch
diff mbox series

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