linux-kernel - Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <35b0dbf2-54a9-23c8-9001-1da7298617b1@csgroup.eu>
Date:   Tue, 5 Sep 2023 07:03:17 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH] powerpc/64e: Fix wrong test in
 __ptep_test_and_clear_young()



Le 05/09/2023 à 06:46, Christophe Leroy a écrit :
> 
> 
> Le 05/09/2023 à 04:36, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@...roup.eu> writes:
>>> Commit 45201c879469 ("powerpc/nohash: Remove hash related code from
>>> nohash headers.") replaced:
>>>
>>>    if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
>>>     return 0;
>>>
>>> By:
>>>
>>>    if (pte_young(*ptep))
>>>     return 0;
>>>
>>> But it should be:
>>>
>>>    if (!pte_young(*ptep))
>>>     return 0;
>>
>>
>> That seems bad :)
>>
>> But I don't know off the top of my head where
>> __ptep_test_and_clear_young() is used, and so what the symptoms could
>> be. Presumably nothing too bad or someone would have noticed?
>>
> 
> The two uses in mm/vmscan.c are as follows:
> 
>      if (!ptep_test_and_clear_young(args->vma, addr, pte + i))
>          VM_WARN_ON_ONCE(true);
> 
> So it seems to be expected to never happen.
> 
> The only useful place it is used seems to be folio_check_references() 
> which is part of the reclaim process. So probably it messes up swap, but 
> to what extent ? ppc64e is for embedded systems. Do embedded systems 
> have swap at all ?
> 
> Also surprising that it is also called from mm/debug_vm_pgtable.c so 
> shouldn't we have noticed earlier ? I'll check if it works.


I confirm CONFIG_DEBUG_VM_PGTABLE on QEMU detects the problem:

debug_vm_pgtable: [debug_vm_pgtable         ]: Validating architecture 
page table helpers
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at mm/debug_vm_pgtable.c:174 
debug_vm_pgtable+0x82c/0xa78
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.5.0-11584-g0ad6bbfc7dab #457
Hardware name: QEMU ppce500 e6500 0x80400020 QEMU e500
NIP:  c0000000010fbe20 LR: c0000000010fbdf8 CTR: 0000000000000000
REGS: c0000000030cb860 TRAP: 0700   Not tainted  (6.5.0-11584-g0ad6bbfc7dab)
MSR:  0000000080029002 <CE,EE,ME>  CR: 44000484  XER: 00000000
IRQMASK: 0
GPR00: c0000000010fbde0 c0000000030cbb00 c0000000010e1200 c00000003f2a9eb8
GPR04: 000033cbee9df000 c0000000039b0ef8 00000039b124028d 0000000000000001
GPR08: 0000000000000000 0000000000000001 c0000000039b0ef8 0000000000000001
GPR12: 0000000024000248 c00000000132e000 c000000000001e38 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: c0000000010da0c4 c00000000112d068 c000000003015920 0000000000200000
GPR28: 0000000010000000 0000000000010000 c00000003f2a9eb8 00000039b124028d
NIP [c0000000010fbe20] debug_vm_pgtable+0x82c/0xa78
LR [c0000000010fbdf8] debug_vm_pgtable+0x804/0xa78
Call Trace:
[c0000000030cbb00] [c0000000010fbde0] debug_vm_pgtable+0x7ec/0xa78 
(unreliable)
[c0000000030cbc70] [c000000000001a38] do_one_initcall+0x68/0x2b8
[c0000000030cbd40] [c0000000010db498] kernel_init_freeable+0x2d0/0x348
[c0000000030cbde0] [c000000000001e64] kernel_init+0x34/0x170
[c0000000030cbe50] [c000000000000594] ret_from_kernel_user_thread+0x14/0x1c
--- interrupt: 0 at 0x0
NIP:  0000000000000000 LR: 0000000000000000 CTR: 0000000000000000
REGS: c0000000030cbe80 TRAP: 0000   Not tainted  (6.5.0-11584-g0ad6bbfc7dab)
MSR:  0000000000000000 <>  CR: 00000000  XER: 00000000
IRQMASK: 0
GPR00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR04: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR28: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
NIP [0000000000000000] 0x0
LR [0000000000000000] 0x0
--- interrupt: 0
Code: 7c7e189e 4b16a3e9 e9410090 e92a0000 792b77e3 40c20010 79296842 
79299800 f92a0000 e9410090 e92a0000 792977e2 <0b090000> 39200000 
f92a0000 e9210090
---[ end trace 0000000000000000 ]---

Christophe