LinuxPPC-Dev Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest
@ 2019-02-07 11:53 Michael Ellerman
  2019-02-07 12:52 ` Segher Boessenkool
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2019-02-07 11:53 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: chandan, daniel, npiggin

Chandan reported that fstests' generic/026 test hit a crash:

  BUG: Unable to handle kernel data access at 0xc00000062ac40000
  Faulting instruction address: 0xc000000000092240
  Oops: Kernel access of bad area, sig: 11 [#1]
  LE SMP NR_CPUS=2048 DEBUG_PAGEALLOC NUMA pSeries
  CPU: 0 PID: 27828 Comm: chacl Not tainted 5.0.0-rc2-next-20190115-00001-g6de6dba64dda #1
  NIP:  c000000000092240 LR: c00000000066a55c CTR: 0000000000000000
  REGS: c00000062c0c3430 TRAP: 0300   Not tainted  (5.0.0-rc2-next-20190115-00001-g6de6dba64dda)
  MSR:  8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 44000842  XER: 20000000
  CFAR: 00007fff7f3108ac DAR: c00000062ac40000 DSISR: 40000000 IRQMASK: 0
  GPR00: 0000000000000000 c00000062c0c36c0 c0000000017f4c00 c00000000121a660
  GPR04: c00000062ac3fff9 0000000000000004 0000000000000020 00000000275b19c4
  GPR08: 000000000000000c 46494c4500000000 5347495f41434c5f c0000000026073a0
  GPR12: 0000000000000000 c0000000027a0000 0000000000000000 0000000000000000
  GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
  GPR20: c00000062ea70020 c00000062c0c38d0 0000000000000002 0000000000000002
  GPR24: c00000062ac3ffe8 00000000275b19c4 0000000000000001 c00000062ac30000
  GPR28: c00000062c0c38d0 c00000062ac30050 c00000062ac30058 0000000000000000
  NIP memcmp+0x120/0x690
  LR  xfs_attr3_leaf_lookup_int+0x53c/0x5b0
  Call Trace:
    xfs_attr3_leaf_lookup_int+0x78/0x5b0 (unreliable)
    xfs_da3_node_lookup_int+0x32c/0x5a0
    xfs_attr_node_addname+0x170/0x6b0
    xfs_attr_set+0x2ac/0x340
    __xfs_set_acl+0xf0/0x230
    xfs_set_acl+0xd0/0x160
    set_posix_acl+0xc0/0x130
    posix_acl_xattr_set+0x68/0x110
    __vfs_setxattr+0xa4/0x110
    __vfs_setxattr_noperm+0xac/0x240
    vfs_setxattr+0x128/0x130
    setxattr+0x248/0x600
    path_setxattr+0x108/0x120
    sys_setxattr+0x28/0x40
    system_call+0x5c/0x70
  Instruction dump:
  7d201c28 7d402428 7c295040 38630008 38840008 408201f0 4200ffe8 2c050000
  4182ff6c 20c50008 54c61838 7d201c28 <7d402428> 7d293436 7d4a3436 7c295040

The instruction dump decodes as:
  subfic  r6,r5,8
  rlwinm  r6,r6,3,0,28
  ldbrx   r9,0,r3
  ldbrx   r10,0,r4	<-

Which shows us doing an 8 byte load from c00000062ac3fff9, which
crosses the page boundary at c00000062ac40000 and faults.

It's not OK for memcmp to read past the end of the source or
destination buffers.

The bug is in the code at the .Lcmp_rest_lt8bytes label. To fix it
test if we have at least 4 bytes to compare and if so do a 4 byte load
and compare. Otherwise, and/or if we have anything left, jump to the
existing code that does byte at a time comparison.

Reported-by: Chandan Rajendra <chandan@linux.ibm.com>
Tested-by: Chandan Rajendra <chandan@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---

v2: Use cmpdi not cmpwi as pointed out by Nick. It's a 64-bit reg so
    it's clearer to use a 64-bit compare, even if we know the value is
    less than 8.

 arch/powerpc/lib/memcmp_64.S | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S
index 844d8e774492..ce4d6ca3a401 100644
--- a/arch/powerpc/lib/memcmp_64.S
+++ b/arch/powerpc/lib/memcmp_64.S
@@ -215,20 +215,29 @@ _GLOBAL_TOC(memcmp)
 	beq	.Lzero
 
 .Lcmp_rest_lt8bytes:
-	/* Here we have only less than 8 bytes to compare with. at least s1
-	 * Address is aligned with 8 bytes.
-	 * The next double words are load and shift right with appropriate
-	 * bits.
+	/*
+	 * Here we have less than 8 bytes left to compare with. We mustn't read
+	 * past the end of either source or dest.
 	 */
-	subfic  r6,r5,8
-	slwi	r6,r6,3
-	LD	rA,0,r3
-	LD	rB,0,r4
-	srd	rA,rA,r6
-	srd	rB,rB,r6
-	cmpld	cr0,rA,rB
+
+	/* If we have less than 4 bytes, just do byte at a time */
+	cmpdi   cr1, r5, 4
+	blt	cr1, .Lshort
+
+	/* Compare 4 bytes */
+	LW	rA,0,r3
+	LW	rB,0,r4
+	cmplw	cr0,rA,rB
 	bne	cr0,.LcmpAB_lightweight
-	b	.Lzero
+
+	/* If we had exactly 4 bytes left, we're done now */
+	beq	cr1, .Lzero
+
+	/* Otherwise do what ever's left a byte at a time */
+	subi	r5, r5, 4
+	addi	r3, r3, 4
+	addi	r4, r4, 4
+	b	.Lshort
 
 .Lnon_zero:
 	mr	r3,rC
-- 
2.20.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest
  2019-02-07 11:53 [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest Michael Ellerman
@ 2019-02-07 12:52 ` Segher Boessenkool
  2019-02-08  6:12   ` Michael Ellerman
  0 siblings, 1 reply; 4+ messages in thread
From: Segher Boessenkool @ 2019-02-07 12:52 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, chandan, npiggin, daniel

On Thu, Feb 07, 2019 at 10:53:13PM +1100, Michael Ellerman wrote:
> Chandan reported that fstests' generic/026 test hit a crash:

> The instruction dump decodes as:
>   subfic  r6,r5,8
>   rlwinm  r6,r6,3,0,28
>   ldbrx   r9,0,r3
>   ldbrx   r10,0,r4	<-
> 
> Which shows us doing an 8 byte load from c00000062ac3fff9, which
> crosses the page boundary at c00000062ac40000 and faults.
> 
> It's not OK for memcmp to read past the end of the source or
> destination buffers.

It's not okay to access memory pages unsolicited.  Reading past the end
is fine per se.


Segher

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest
  2019-02-07 12:52 ` Segher Boessenkool
@ 2019-02-08  6:12   ` Michael Ellerman
  2019-02-08 15:50     ` Segher Boessenkool
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2019-02-08  6:12 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: linuxppc-dev, chandan, npiggin, daniel

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Feb 07, 2019 at 10:53:13PM +1100, Michael Ellerman wrote:
>> Chandan reported that fstests' generic/026 test hit a crash:
>
>> The instruction dump decodes as:
>>   subfic  r6,r5,8
>>   rlwinm  r6,r6,3,0,28
>>   ldbrx   r9,0,r3
>>   ldbrx   r10,0,r4	<-
>> 
>> Which shows us doing an 8 byte load from c00000062ac3fff9, which
>> crosses the page boundary at c00000062ac40000 and faults.
>> 
>> It's not OK for memcmp to read past the end of the source or
>> destination buffers.
>
> It's not okay to access memory pages unsolicited.  Reading past the end
> is fine per se.

Yeah I guess that's true.

Things like KASAN/valgrind probably disagree, but KASAN at least
overrides memcmp AIUI.

I guess I feel better about it not reading past the end of the buffers,
but maybe I'm being paranoid.

The other complication is we support multiple page sizes, so detecting a
page boundary is more complicated than it could be.

So I guess I'm inclined to stick with this approach, but I can update
the change log.

cheers

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest
  2019-02-08  6:12   ` Michael Ellerman
@ 2019-02-08 15:50     ` Segher Boessenkool
  0 siblings, 0 replies; 4+ messages in thread
From: Segher Boessenkool @ 2019-02-08 15:50 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev, chandan, npiggin, daniel

On Fri, Feb 08, 2019 at 05:12:21PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Thu, Feb 07, 2019 at 10:53:13PM +1100, Michael Ellerman wrote:
> >> Chandan reported that fstests' generic/026 test hit a crash:
> >
> >> The instruction dump decodes as:
> >>   subfic  r6,r5,8
> >>   rlwinm  r6,r6,3,0,28
> >>   ldbrx   r9,0,r3
> >>   ldbrx   r10,0,r4	<-
> >> 
> >> Which shows us doing an 8 byte load from c00000062ac3fff9, which
> >> crosses the page boundary at c00000062ac40000 and faults.
> >> 
> >> It's not OK for memcmp to read past the end of the source or
> >> destination buffers.
> >
> > It's not okay to access memory pages unsolicited.  Reading past the end
> > is fine per se.
> 
> Yeah I guess that's true.
> 
> Things like KASAN/valgrind probably disagree, but KASAN at least
> overrides memcmp AIUI.
> 
> I guess I feel better about it not reading past the end of the buffers,
> but maybe I'm being paranoid.

Sure, and that may be the best thing to do in the kernel.  OTOH, newer GCC
will inline many mem* for powerpc, and it will access past the end of
strings and buffers (but not past 4kB boundaries).

> The other complication is we support multiple page sizes, so detecting a
> page boundary is more complicated than it could be.

Yeah.

> So I guess I'm inclined to stick with this approach, but I can update
> the change log.

Thanks!  I mentioned it because this was the bug that was hit here: reading
past the end had no ill effect (as far as we know), but accessing the wrong
page did :-)


Segher

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 11:53 [PATCH v2] powerpc/64: Fix memcmp reading past the end of src/dest Michael Ellerman
2019-02-07 12:52 ` Segher Boessenkool
2019-02-08  6:12   ` Michael Ellerman
2019-02-08 15:50     ` Segher Boessenkool

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox