Message ID | 20190207115313.31078-1-mpe@ellerman.id.au (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] powerpc/64: Fix memcmp reading past the end of src/dest | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 41 lines checked |
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
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
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
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