Re: [qemu-s390x] [PATCH v1 2/2] s390x/tcg: Store only the necessary amount of doublewords for STFLE

[Stefan Liebler]

On 6/3/19 12:45 PM, David Hildenbrand wrote:
> On 03.06.19 12:38, Stefan Liebler wrote:
> > On 5/31/19 4:56 PM, David Hildenbrand wrote:
> > > The PoP (z14, 7-382) says:
> > >       Doublewords to the right of the doubleword in which the
> > >       highest-numbered facility bit is assigned for a model
> > >       may or may not be stored.
> > >
> > > However, stack protection in certain binaries can't deal with that.
> > > "gzip" example code:
> > >
> > > f1b4:       a7 08 00 03             lhi     %r0,3
> > > f1b8:       b2 b0 f0 a0             stfle   160(%r15)
> > > f1bc:       e3 20 f0 b2 00 90       llgc    %r2,178(%r15)
> > > f1c2:       c0 2b 00 00 00 01       nilf    %r2,1
> > > f1c8:       b2 4f 00 10             ear     %r1,%a0
> > > f1cc:       b9 14 00 22             lgfr    %r2,%r2
> > > f1d0:       eb 11 00 20 00 0d       sllg    %r1,%r1,32
> > > f1d6:       b2 4f 00 11             ear     %r1,%a1
> > > f1da:       d5 07 f0 b8 10 28       clc     184(8,%r15),40(%r1)
> > > f1e0:       a7 74 00 06             jne     f1ec <file_read@@Base+0x1bc>
> > > f1e4:       eb ef f1 30 00 04       lmg     %r14,%r15,304(%r15)
> > > f1ea:       07 fe                   br      %r14
> > > f1ec:       c0 e5 ff ff 9d 6e       brasl   %r14,2cc8 <address@hidden>
> > >
> > > In QEMU, we currently have:
> > >       max_bytes = 24
> > > the code asks for (3 + 1) doublewords == 32 bytes.
> > >
> > > If we write 32 bytes instead of only 24, and return "2 + 1" doublewords
> > > ("one less than the number of doulewords needed to contain all of the
> > >    facility bits"), the example code detects a stack corruption.
> > >
> > > In my opinion, the code is wrong. However, it seems to work fine on
> > > real machines. So let's limit storing to the minimum of the requested
> > > and the maximum doublewords.
> > Hi David,
> >
> > Thanks for catching this. I've reported the "gzip" example to Ilya and
> > indeed, r0 is setup too large. He will fix it in gzip.
> >
> > You've mentioned, that this is detected in certain binaries.
> > Can you please share those occurrences.
>
> Hi Stafan,
>
> thanks for your reply.
>
> I didn't track all occurrences, it *could* be that it was only gzip in
> the background making other processes fail.
>
> For example, the systemd "vitual console setup" unit failed, too, which
> was fixed by this change.
At least "objdump -d /usr/lib/systemd/systemd-vconsole-setup" does not 
contain the stfle instruction, but "ldd 
/usr/lib/systemd/systemd-vconsole-setup" is showing libz.so which could 
also contain Ilya's patches with the stfle instruction (I assume there 
is the same bug as in gzip). But I have no idea if libz is really called.
> I also remember, seeing segfaults in rpmbuild, for example. They only
> "changed" with this fix - I m still chasing different errors. :)
The same applies for rpmbuild.
> You mentioned "He will fix it in gzip", so I assume this is a gzip issue
> and not a gcc/glibc/whatever toolchain issue?
Yes, this is a gzip bug. r0 was initialized with:
(sizeof(array-on-stack) / 8)
instead of:
(sizeof(array-on-stack) / 8) - 1

Ilya will fix it in gzip and zlib.
@Ilya: Please correct me if I'm wrong.