RE: -Warray-bounds fun again

From: David Laight <David.Laight@ACULAB.COM>
To: 'Linus Torvalds' <torvalds@linux-foundation.org>,
	Sven Schnelle <svens@linux.ibm.com>
Cc: Kees Cook <keescook@chromium.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"linux-hardening@vger.kernel.org"
	<linux-hardening@vger.kernel.org>,
	"krebbel@linux.ibm.com" <krebbel@linux.ibm.com>,
	Ilya Leoshkevich <iii@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>
Subject: RE: -Warray-bounds fun again
Date: Fri, 22 Apr 2022 08:24:37 +0000	[thread overview]
Message-ID: <fd6b4de14d944680b5a5674edfe34654@AcuMS.aculab.com> (raw)
In-Reply-To: <CAHk-=wgv4NLtEowsmX+0jq_nBWXFp8jtruX6U3SDm52N=ftkgg@mail.gmail.com>

From: Linus Torvalds
> Sent: 21 April 2022 17:11
> 
> On Thu, Apr 21, 2022 at 7:02 AM Sven Schnelle <svens@linux.ibm.com> wrote:
> >
> > The obvious 'fix' is to use absolute_pointer():
> >
> > #define S390_lowcore (*((struct lowcore *)absolute_pointer(0)))
> >
> > That makes the warning go away, but unfortunately the compiler no longer
> > knows that the memory access is fitting into a load/store with a 12 bit
> > displacement.
> 
> In the gcc bugzilla for us needing to do these games:
> 
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> 
> one of the suggestions was "I recommend suppressing the warning either
> by #pragma GCC diagnostic or by making the pointer volatile".
> 
> But READ_ONCE() should already be doing that volatile thing, so that
> suggestion may not work with gcc-12 any more.
> 
> It is *possible* that gcc-12 has now special-cased the very special
> issue of a cast of the constant zero. That is how NULL was
> traditionally defined.
> 
> So just out of a perverse curiosity, what happens if you do something like this:
> 
>    #define S390_lowcore_end ((struct lowcore *)sizeof(struct lowcore))
>    #define S390_lowcore (S390_lowcore_end[-1])
> 
> instead? It should get the same value in the end, but it doesn't have
> that special case of "cast an integer constant 0 to a pointer".
> 
> I suspect it probably doesn't help, because gcc will still see "oh,
> you're basing this off address zero".
> 
> Another thing to try might be to remove the initial 16 bytes of
> padding from 'struct lowcore' (it looks like the first 20 bytes are
> not used - so leave 4 bytes of padding still), and use
> 
>    #define S390_lowcore (*((struct lowcore_nopad *)16))
> 
> instead. Then gcc will never see that 0, and hopefully the "he's
> accessing based off a NULL pointer" logic will go away.
> 
> Because right now, our absolute_pointer() protection against this
> horrible gcc mis-feature is literally based on hiding the value from
> the compiler with an inline asm, and by virtue of hiding the value
> then yes, gcc will have to go through a register base pointer and
> cannot see that it fits in 12 bits.

I think you might be mixing up two problems.

Accessing ((foo *)0)->member is problematic because NULL might not be zero.
In which case an unexpected address is generated.
I think this is why clang really doesn't like you doing that.
Using ((foo *)(sizeof (foo))[-1].member might get around that.

I suspect the array bounds issue is caused by gcc using a size of 0
for 'I don't know the size' and then assuming it is real size later on.
That seems like a real gcc bug.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)