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

[Stefan Liebler <stli@linux.ibm.com>]

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 <__stack_chk_fail@plt>
> 
> 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.
Perhaps on future machines with further facility bits, stfle will write 
beyond the provided doubleword-array on a real machine or in qemu.

Bye
Stefan

> 
> Cc: Stefan Liebler <stli@linux.ibm.com>
> Cc: Andreas Krebbel <Andreas.Krebbel@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/misc_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 34476134a4..b561c5781b 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -678,7 +678,7 @@ uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr)
>   
>       prepare_stfl();
>       max_bytes = ROUND_UP(used_stfl_bytes, 8);
> -    for (i = 0; i < count_bytes; ++i) {
> +    for (i = 0; i < MIN(count_bytes, max_bytes); ++i) {
>           cpu_stb_data_ra(env, addr + i, stfl_bytes[i], ra);
>       }
>   
>