linux-kernel - Re: [RFC][PATCH 07/22] x86,extable: Extend extable functionality

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]

Date:   Fri, 5 Nov 2021 21:15:57 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, jpoimboe@...hat.com,
        mark.rutland@....com, dvyukov@...gle.com, pbonzini@...hat.com,
        mbenes@...e.cz
Subject: Re: [RFC][PATCH 07/22] x86,extable: Extend extable functionality

On Fri, Nov 05, 2021 at 07:47:22PM +0000, Sean Christopherson wrote:
> On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> > On Fri, Nov 05, 2021 at 07:17:20PM +0000, Sean Christopherson wrote:

> > C99 is sodding daft wrt signed values. That's why we force -fwrapv and
> > say signed is 2s complement and expect sanity.
> 
> FWIW, -fwrapv was supplanted by -fno-strict-overflow back in 2009 by commit
> a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x").
> But I don't think that matters because AFAICT both apply only to "addition,
> subtraction and multiplication".

Right... I think i remember running into that before. I think I yelled
at people at the time for that not being very consistent. If we
explicitly state we want 2s complement, it had damn well be everywhere.

> > > gcc-10 generates a bare "shr", i.e. doesn't special case negative values, and "shr"
> > > is explicitly defined as an unsigned divide.
> > 
> > We hard rely on signed shift right to preserve sign all over the place,
> > how come it goes sideways here? Lemme go stare at asm...
> 
> Huh.  TIL there's actually a use case for this.

Yeah, the canonical pattern for sign extending an n-bit int is:

	(int)(x << (32-n)) >> (32-n)

I think we have macros for that somehwere, but I can never remember what
they're called.

> Anyways, I think the issue is that EX_IMM_MASK is being interpreted as an unsigned
> value by the compiler.  Which I think is legal for a 64-bit kernel?  Because bit 63
> is the MSB, not bit 31.  E.g. this
> 
> 	FIELD_GET((int)EX_IMM_MASK, e->type)
> 
> generates SAR instead of SHR.

Bah, you're right. The compiler is free to interpret a hex constant as
either. At the same time there's only an unsigned suffix, so while we
can force it unsigned, we can't explicitly construct a signed hex
constant.

That's really unfortunate that is... 6.4.4.1 item 5 of the C99 spec
covers this gem :-( I suppose I'll go stick that (int) cast in the
EX_IMM_MASK definition or something.