linux-kernel - Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and 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: <20220831224522.GX25951@gate.crashing.org>
Date:   Wed, 31 Aug 2022 17:45:22 -0500
From:   Segher Boessenkool <segher@...nel.crashing.org>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Nicholas Piggin <npiggin@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        Zhouyi Zhou <zhouzhouyi@...il.com>
Subject: Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

Hi!

On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
> > On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
> >>> This is still slightly concerning to me. Is there any guarantee that the
> >>> compiler would not use a different sequence for the address here?
> >>>
> >>> Maybe explicit r13 is required.
> >>>
> >>
> >> local_paca is defined as:
> >>
> >> 	register struct paca_struct *local_paca asm("r13");

And this is in global scope, making it a global register variable.

> >> Why would the compiler use another register ?
> > 
> > Hopefully it doesn't. Is it guaranteed that it won't?

Yes, this is guaranteed.

For a local register variable this is guaranteed only for operands to an
extended inline asm; any other access to the variable does not have to
put it in the specified register.

But this is a global register variable.  The only thing that would make
this crash and burn is if *any* translation unit did not see this
declaration: it could then use r13 (if that was allowed by the ABI in
effect, heh).

> > I'm sure Segher will be delighted with the creative asm in __do_IRQ
> > and call_do_irq :) *Grabs popcorn*

All that %% is blinding, yes.

Inline tabs are bad taste.

Operand names instead of numbers are great for obfuscation, and nothing
else -- unless you have more than four or five operands, in which case
you have bigger problems already.

Oh, this function is a good example of proper use of local register asm,
btw.

Comments like "// Inputs" are just harmful.  As is the "creative"
indentation here.  Both harm readability and do not help understanding
in any other way either.

The thing about inline asm is the smallest details change meaning of the
whole, it is a very harsh environment, you are programming both in C and
directly assembler code as well, and things have to be valid for both,
although on the other hand there is almost no error checking.  Keeping
it small, simple, readable is paramount.

The rules for using inline asm:

0) Do no use inline asm.
1) Use extended asm, unless you know all differences with basic asm, and
   you know you want that.  And if you answer "yes I do" to the latter,
   you answered wrong to the former.
2) Do not use toplevel asm.
3) Do no use inline asm.
4) Do no use inline asm.
5) Do no use inline asm.

Inline asm is a very powerful escape hatch.  Like all emergency exits,
you should not use them if you do not need them!  :-)

But, you are talking about the function calling and the frame change I
bet :-)  Both of these are only okay because everything is back as it
was when this (single!) asm is done, and the state created is perfectly
fine (this is very dependent on exact ABI used, etc.)

I would have used real assembler code here (in a .s file).  But there
probably are reasons to do things this way, performance probably?

Segher