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

[David Hildenbrand <david@redhat.com>]

On 03.06.19 11:06, 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.
> 
> Cc: Stefan Liebler <stli@linux.ibm.com>
> Cc: Andreas Krebbel <Andreas.Krebbel@de.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/misc_helper.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 34476134a4..10aa617cf9 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -678,7 +678,13 @@ 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) {
> +
> +    /*
> +     * The PoP says that doublewords beyond the highest-numbered facility
> +     * bit may or may not be stored.  However, existing hardware appears to
> +     * not store the words, and existing software depend on that.
> +     */
> +    for (i = 0; i < MIN(count_bytes, max_bytes); ++i) {
>          cpu_stb_data_ra(env, addr + i, stfl_bytes[i], ra);
>      }
>  
> 

Not intended to be included in this series, please ignore :)

-- 

Thanks,

David / dhildenb