linux-kernel - Re: [PATCH linux-next] powerpc: disable sanitizer in irq_soft_mask

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7f5cff06-903f-f823-db10-ed6d2aafba23@csgroup.eu>
Date:   Tue, 23 Aug 2022 05:53:50 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Zhouyi Zhou <zhouzhouyi@...il.com>
CC:     "mpe@...erman.id.au" <mpe@...erman.id.au>,
        "npiggin@...il.com" <npiggin@...il.com>,
        "atrajeev@...ux.vnet.ibm.com" <atrajeev@...ux.vnet.ibm.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lance@...osl.org" <lance@...osl.org>,
        "paulmck@...nel.org" <paulmck@...nel.org>,
        "rcu@...r.kernel.org" <rcu@...r.kernel.org>
Subject: Re: [PATCH linux-next] powerpc: disable sanitizer in
 irq_soft_mask_set



Le 23/08/2022 à 03:43, Zhouyi Zhou a écrit :
> On Mon, Aug 22, 2022 at 2:04 PM Christophe Leroy
> <christophe.leroy@...roup.eu> wrote:
>>
>>
>>
>> Le 21/08/2022 à 03:00, Zhouyi Zhou a écrit :
>>> In ppc, compiler based sanitizer will generate instrument instructions
>>> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
>>>
>>>      0xc000000000295cb0 <+0>:  addis   r2,r12,774
>>>      0xc000000000295cb4 <+4>:  addi    r2,r2,16464
>>>      0xc000000000295cb8 <+8>:  mflr    r0
>>>      0xc000000000295cbc <+12>: bl      0xc00000000008bb4c <mcount>
>>>      0xc000000000295cc0 <+16>: mflr    r0
>>>      0xc000000000295cc4 <+20>: std     r31,-8(r1)
>>>      0xc000000000295cc8 <+24>: addi    r3,r13,2354
>>>      0xc000000000295ccc <+28>: mr      r31,r13
>>>      0xc000000000295cd0 <+32>: std     r0,16(r1)
>>>      0xc000000000295cd4 <+36>: stdu    r1,-48(r1)
>>>      0xc000000000295cd8 <+40>: bl      0xc000000000609b98 <__asan_store1+8>
>>>      0xc000000000295cdc <+44>: nop
>>>      0xc000000000295ce0 <+48>: li      r9,1
>>>      0xc000000000295ce4 <+52>: stb     r9,2354(r31)
>>>      0xc000000000295ce8 <+56>: addi    r1,r1,48
>>>      0xc000000000295cec <+60>: ld      r0,16(r1)
>>>      0xc000000000295cf0 <+64>: ld      r31,-8(r1)
>>>      0xc000000000295cf4 <+68>: mtlr    r0
>>>
>>> If there is a context switch before "stb     r9,2354(r31)", r31 may
>>> not equal to r13, in such case, irq soft mask will not work.
>>>
>>> This patch disable sanitizer in irq_soft_mask_set.
>>
>> Well spotted, thanks.
> Thank Christophe for reviewing my patch and your guidance!
>>
>> You should add:
>>
>> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
> OK, I will do it!
>>
>> By the way, I think the READ_ONCE() likely has the same issue so you
>> should fix irq_soft_mask_return() at the same time.
> Yes, after disassembling irq_soft_mask_return, she has the same issue
> as irq_soft_mask_set.
> 
> In addition, I read hw_irq.h by naked eye to search for removed inline
> assembly one by one,
> I found another place that we could possible enhance (Thank Paul E.
> McKenny for teaching me use git blame ;-)):
> 077fc62b2b66a("powerpc/irq: remove inline assembly in hard_irq_disable macro")
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -282,9 +282,7 @@ static inline bool pmi_irq_pending(void)
>          flags = irq_soft_mask_set_return(IRQS_ALL_DISABLED);            \
>          local_paca->irq_happened |= PACA_IRQ_HARD_DIS;                  \
>          if (!arch_irqs_disabled_flags(flags)) {                         \
> -               asm ("stdx %%r1, 0, %1 ;"                               \
> -                    : "=m" (local_paca->saved_r1)                      \
> -                    : "b" (&local_paca->saved_r1));                    \
> +               WRITE_ONCE(local_paca->saved_r1, current_stack_pointer);\
>                  trace_hardirqs_off();                                   \
>          }                                                               \
>   } while(0)
> I wrap the macro hard_irq_disable into a test function and disassemble
> it, she has the above issue too:
> (gdb) disassemble test002
> Dump of assembler code for function test002:
...
> Could we rewrite this macro into a static inline function as
> irq_soft_mask_set does, and disable sanitizer for it?

Yes we could try to do that, hoping it will not bring too much troubles 
with circular header inclusion.

Another possible approach could be to create a WRITE_ONCE_NOCHECK() more 
or less similar to existing READ_ONCE_NOCHECK().

We could also change READ_ONCE_NOCHECK() to accept bytes size then use 
it for irq_soft_mask_return() .

Christophe