[PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Michael Ellerman]

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 if that would cross a page boundary, because we
don't know that the next page is mapped.

As pointed out by Segher, we can read past the end of the source or
destination as long as we don't cross a 4K boundary, because that's
our minimum page size on all platforms.

The bug is in the code at the .Lcmp_rest_lt8bytes label. When we get
there we know that s1 is 8-byte aligned and we have at least 1 byte to
read, so a single 8-byte load won't read past the end of s1 and cross
a page boundary.

But we have to be more careful with s2. So check if it's within 8
bytes of a 4K boundary and if so go to the byte-by-byte loop.

Fixes: 2d9ee327adce ("powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()")
Cc: stable@vger.kernel.org # v4.19+
Reported-by: Chandan Rajendra <chandan@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/lib/memcmp_64.S | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

v3: Just check if we're crossing a 4K boundary.

Oops, I wrote this a while back but forgot to send v3.

diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S
index 844d8e774492..f6554ebebeb5 100644
--- a/arch/powerpc/lib/memcmp_64.S
+++ b/arch/powerpc/lib/memcmp_64.S
@@ -215,11 +215,20 @@ _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 to compare. At least s1 is aligned to
+	 * 8 bytes, but s2 may not be. We must make sure s2 + 8 doesn't cross a
+	 * page boundary, otherwise we might read past the end of the buffer and
+	 * trigger a page fault. We use 4K as the conservative minimum page
+	 * size. If we detect that case we go to the byte-by-byte loop.
+	 *
+	 * Otherwise the next double word is loaded from s1 and s2, and shifted
+	 * right to compare the appropriate bits.
 	 */
+	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff
+	cmpdi	r6,0xff8
+	bgt	.Lshort
+
 	subfic  r6,r5,8
 	slwi	r6,r6,3
 	LD	rA,0,r3
-- 
2.20.1

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Segher Boessenkool]

On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
>  .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 to compare. At least s1 is aligned to
> +	 * 8 bytes, but s2 may not be. We must make sure s2 + 8 doesn't cross a

"s2 + 7"?  The code is fine though (bgt, not bge).

> +	 * page boundary, otherwise we might read past the end of the buffer and
> +	 * trigger a page fault. We use 4K as the conservative minimum page
> +	 * size. If we detect that case we go to the byte-by-byte loop.
> +	 *
> +	 * Otherwise the next double word is loaded from s1 and s2, and shifted
> +	 * right to compare the appropriate bits.
>  	 */
> +	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff

You can just write
  rlwinm r6,r4,0,0x0fff
if that is clearer?  Or do you still want a comment with that :-)

> +	cmpdi	r6,0xff8
> +	bgt	.Lshort

Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>


Segher

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Chandan Rajendra]

On Friday, March 22, 2019 6:07:24 PM IST Michael Ellerman wrote:
> 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 if that would cross a page boundary, because we
> don't know that the next page is mapped.
> 
> As pointed out by Segher, we can read past the end of the source or
> destination as long as we don't cross a 4K boundary, because that's
> our minimum page size on all platforms.
> 
> The bug is in the code at the .Lcmp_rest_lt8bytes label. When we get
> there we know that s1 is 8-byte aligned and we have at least 1 byte to
> read, so a single 8-byte load won't read past the end of s1 and cross
> a page boundary.
> 
> But we have to be more careful with s2. So check if it's within 8
> bytes of a 4K boundary and if so go to the byte-by-byte loop.
> 
> Fixes: 2d9ee327adce ("powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()")
> Cc: stable@vger.kernel.org # v4.19+
> Reported-by: Chandan Rajendra <chandan@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

For unknown reasons, I am unable to recreate this bug on the unmodified
next-20190115 which was the original kernel I had found this bug on.

FWIW, I have executed generic/026 on a next-20190115 kernel with this patch
applied and I wasn't able to recreate the bug. Hence,

Tested-by: Chandan Rajendra <chandan@linux.ibm.com>

-- 
chandan

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Chandan Rajendra]

On Friday, March 22, 2019 6:07:24 PM IST Michael Ellerman wrote:
> 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 if that would cross a page boundary, because we
> don't know that the next page is mapped.
> 
> As pointed out by Segher, we can read past the end of the source or
> destination as long as we don't cross a 4K boundary, because that's
> our minimum page size on all platforms.
> 
> The bug is in the code at the .Lcmp_rest_lt8bytes label. When we get
> there we know that s1 is 8-byte aligned and we have at least 1 byte to
> read, so a single 8-byte load won't read past the end of s1 and cross
> a page boundary.
> 
> But we have to be more careful with s2. So check if it's within 8
> bytes of a 4K boundary and if so go to the byte-by-byte loop.
> 
> Fixes: 2d9ee327adce ("powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()")
> Cc: stable@vger.kernel.org # v4.19+
> Reported-by: Chandan Rajendra <chandan@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

For unknown reasons, I am unable to recreate this bug on the unmodified
next-20190115 which was the original kernel I had found this bug on.

FWIW, I have executed generic/026 on a next-20190115 kernel with this patch
applied and I wasn't able to recreate the bug. Hence,

Tested-by: Chandan Rajendra <chandan@linux.ibm.com>

-- 
chandan

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Michael Ellerman]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
>>  .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 to compare. At least s1 is aligned to
>> +	 * 8 bytes, but s2 may not be. We must make sure s2 + 8 doesn't cross a
>
> "s2 + 7"?  The code is fine though (bgt, not bge).

Duh, thanks for catching it.

>> +	 * page boundary, otherwise we might read past the end of the buffer and
>> +	 * trigger a page fault. We use 4K as the conservative minimum page
>> +	 * size. If we detect that case we go to the byte-by-byte loop.
>> +	 *
>> +	 * Otherwise the next double word is loaded from s1 and s2, and shifted
>> +	 * right to compare the appropriate bits.
>>  	 */
>> +	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff
>
> You can just write
>   rlwinm r6,r4,0,0x0fff

> if that is clearer?  Or do you still want a comment with that :-)

I don't think it's clearer doing a rotate of zero bits :)

And yeah I'd probably still leave the comment, so I'm inclined to stick
with the clrldi?

Would be nice if the assembler could support:

	andi	r6, r4, 0x0fff

And turn it into the rlwinm, or rldicl :)

>> +	cmpdi	r6,0xff8
>> +	bgt	.Lshort
>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>

I'll fixup the comment. Thanks.

cheers

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Segher Boessenkool]

On Mon, Mar 25, 2019 at 11:33:56PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
> >> +	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff
> >
> > You can just write
> >   rlwinm r6,r4,0,0x0fff
> 
> > if that is clearer?  Or do you still want a comment with that :-)
> 
> I don't think it's clearer doing a rotate of zero bits :)
> 
> And yeah I'd probably still leave the comment, so I'm inclined to stick
> with the clrldi?

I always have to think what the clrldi etc. do exactly, while with rlwinm
it is obvious.  But yeah this may be different for other people who are
used to different idiom.

> Would be nice if the assembler could support:
> 
> 	andi	r6, r4, 0x0fff
> 
> And turn it into the rlwinm, or rldicl :)

The extended mnemonics are *simple*, *one-to-one* mappings.  Having
"andi. 6,4,0x0f0f" a valid insn, but an extended mnemonic "andi 6,4,0x0f0f"
that is not (and the other way around for say 0xff0000ff) would violate that.

You could do some assembler macro, that can also expand to multiple insns
where that is useful.  Also one for loading constants, etc.  The downside
to that is you often do care how many insns are generated.

Instead you could do a macro for only those cases that can be done with *one*
insn.  But that then is pretty restricted in use, and people have to learn
what values are valid.

I don't see a perfect solution.


Segher

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Michael Ellerman]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Mar 25, 2019 at 11:33:56PM +1100, Michael Ellerman wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
>> >> +	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff
>> >
>> > You can just write
>> >   rlwinm r6,r4,0,0x0fff
>> 
>> > if that is clearer?  Or do you still want a comment with that :-)
>> 
>> I don't think it's clearer doing a rotate of zero bits :)
>> 
>> And yeah I'd probably still leave the comment, so I'm inclined to stick
>> with the clrldi?
>
> I always have to think what the clrldi etc. do exactly, while with rlwinm
> it is obvious.  But yeah this may be different for other people who are
> used to different idiom.

Interesting, I'm the opposite. You know ppc assembler better than me so
I guess I just need to spend more time on it and embrace the zen of the
rotate instructions.

>> Would be nice if the assembler could support:
>> 
>> 	andi	r6, r4, 0x0fff
>> 
>> And turn it into the rlwinm, or rldicl :)
>
> The extended mnemonics are *simple*, *one-to-one* mappings.

It would still be simple and 1:1, but would only be valid for certain
constants :)

> Having "andi. 6,4,0x0f0f" a valid insn, but an extended mnemonic "andi 6,4,0x0f0f"
> that is not (and the other way around for say 0xff0000ff) would violate that.

I agree that's a bit of a foot gun.

I'm not sure it's vastly more hostile though than `andi. 6,4,0xffff`
being valid but `andi. 6,4,0x1ffff` being not valid.

The assembler could print a nice error saying you need to use a
contiguous mask. And I mean how often do you andi. with a mask that
isn't contiguous?

> You could do some assembler macro, that can also expand to multiple insns
> where that is useful.  Also one for loading constants, etc.  The downside
> to that is you often do care how many insns are generated.
>
> Instead you could do a macro for only those cases that can be done with *one*
> insn.  But that then is pretty restricted in use, and people have to learn
> what values are valid.
>
> I don't see a perfect solution.

Yeah, I guess a new `andi` instruction is the only real answer :)

cheers

Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Segher Boessenkool]

On Tue, Mar 26, 2019 at 08:18:07PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Mon, Mar 25, 2019 at 11:33:56PM +1100, Michael Ellerman wrote:
> >> Segher Boessenkool <segher@kernel.crashing.org> writes:
> >> > On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
> >> >> +	clrldi	r6,r4,(64-12)	// r6 = r4 & 0xfff
> >> >
> >> > You can just write
> >> >   rlwinm r6,r4,0,0x0fff
> >> 
> >> > if that is clearer?  Or do you still want a comment with that :-)
> >> 
> >> I don't think it's clearer doing a rotate of zero bits :)
> >> 
> >> And yeah I'd probably still leave the comment, so I'm inclined to stick
> >> with the clrldi?
> >
> > I always have to think what the clrldi etc. do exactly, while with rlwinm
> > it is obvious.  But yeah this may be different for other people who are
> > used to different idiom.
> 
> Interesting, I'm the opposite. You know ppc assembler better than me so
> I guess I just need to spend more time on it and embrace the zen of the
> rotate instructions.

There is only one rlwinm instruction, but there are 9 extended mnemonics
for it.  (And similarly for the 64-bit ops, where there are 3 basic
mnemonics but 9 extended mnemonics).  Of course I use {s,rot}{l,r}{w,d}i,
those are actually *simpler* than rlwinm / rldic{,l,r}, but not the other
stuff.

It may also be because the disassembler doesn't show these things.  Dunno.

> I'm not sure it's vastly more hostile though than `andi. 6,4,0xffff`
> being valid but `andi. 6,4,0x1ffff` being not valid.

But that is the same for *all* UIMM insns.

> Yeah, I guess a new `andi` instruction is the only real answer :)

Too bad there is no space in the opcode map for it, already not in the
original POWER architecture (where this is "andil.") :-/


Segher

Re: [v3] powerpc/64: Fix memcmp reading past the end of src/dest

[Michael Ellerman]

On Fri, 2019-03-22 at 12:37:24 UTC, Michael Ellerman wrote:
> 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 if that would cross a page boundary, because we
> don't know that the next page is mapped.
> 
> As pointed out by Segher, we can read past the end of the source or
> destination as long as we don't cross a 4K boundary, because that's
> our minimum page size on all platforms.
> 
> The bug is in the code at the .Lcmp_rest_lt8bytes label. When we get
> there we know that s1 is 8-byte aligned and we have at least 1 byte to
> read, so a single 8-byte load won't read past the end of s1 and cross
> a page boundary.
> 
> But we have to be more careful with s2. So check if it's within 8
> bytes of a 4K boundary and if so go to the byte-by-byte loop.
> 
> Fixes: 2d9ee327adce ("powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()")
> Cc: stable@vger.kernel.org # v4.19+
> Reported-by: Chandan Rajendra <chandan@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> Tested-by: Chandan Rajendra <chandan@linux.ibm.com>
> Tested-by: Chandan Rajendra <chandan@linux.ibm.com>

Applied to powerpc fixes.

https://git.kernel.org/powerpc/c/d9470757398a700d9450a43508000bcf

cheers