Re: [PATCH 07/14] target/s390x: Fix assertion failure in VFMIN/VFMAX with reserved type

[David Hildenbrand]

On 19.07.23 11:34, Ilya Leoshkevich wrote:
> On Wed, 2023-07-19 at 10:40 +0200, David Hildenbrand wrote:
> > On 18.07.23 23:21, Ilya Leoshkevich wrote:
> > > Passing reserved type to VFMIN/VFMAX causes an assertion failure in
> > > vfmin_res() and vfmax_res(). These instructions should raise a
> > > specification exception in this case.
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Fixes: da4807527f3b ("s390x/tcg: Implement VECTOR FP
> > > (MAXIMUM|MINIMUM)")
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >    target/s390x/tcg/vec_fpu_helper.c | 24 +++++++++++++++---------
> > >    1 file changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/target/s390x/tcg/vec_fpu_helper.c
> > > b/target/s390x/tcg/vec_fpu_helper.c
> > > index 75cf605b9f4..f1671679879 100644
> > > --- a/target/s390x/tcg/vec_fpu_helper.c
> > > +++ b/target/s390x/tcg/vec_fpu_helper.c
> > > @@ -915,7 +915,7 @@ static void vfminmax32(S390Vector *v1, const
> > > S390Vector *v2,
> > >            float32 b = s390_vec_read_float32(v3, i);
> > >            float32 result;
> > >    
> >
> > Why not check for invalid types once first and leave the rest of that
> > function alone?
> >
> > diff --git a/target/s390x/tcg/vec_fpu_helper.c
> > b/target/s390x/tcg/vec_fpu_helper.c
> > index 75cf605b9f..e0b2a78632 100644
> > --- a/target/s390x/tcg/vec_fpu_helper.c
> > +++ b/target/s390x/tcg/vec_fpu_helper.c
> > @@ -910,6 +910,11 @@ static void vfminmax32(S390Vector *v1, const
> > S390Vector *v2,
> >        S390Vector tmp = {};
> >        int i;
> >    
> > +    if (type > S390_MINMAX_TYPE_F) {
> > +        tcg_s390_program_interrupt(env, PGM_SPECIFICATION, retaddr);
> > +    }
> > +
> >        for (i = 0; i < 4; i++) {
> >            float32 a = s390_vec_read_float32(v2, i);
> >            float32 b = s390_vec_read_float32(v3, i);
>
> I have taken another look, and turns out there already is:
>
> static DisasJumpType op_vfmax(DisasContext *s, DisasOps *o)
> {
>      ...
>
>      if (m6 == 5 || m6 == 6 || m6 == 7 || m6 > 13) {
>          gen_program_exception(s, PGM_SPECIFICATION);
>          return DISAS_NORETURN;
>      }
>
> What the fuzzer has found was the m6 == 13 case, so only a small
> adjustment is needed.

Oh, good!

--
Cheers,

David / dhildenb