Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed

[Greg Kurz]

On Fri, 17 Jan 2020 15:46:57 +1000
David Gibson <address@hidden> wrote:

> On Thu, Jan 16, 2020 at 04:34:06PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Greg,
> > 
> > On 1/16/20 4:05 PM, Greg Kurz wrote:
> > > Most of the option vector helpers have assertions to check their
> > > arguments aren't null. The guest can provide an arbitrary address
> > > for the CAS structure that would result in such null arguments.
> > > Fail CAS with H_PARAMETER instead of aborting QEMU.
> > > 
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---
> > >   hw/ppc/spapr_hcall.c |    9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index 84e1612595bb..051869ae20ec 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -1701,9 +1701,18 @@ static target_ulong 
> > > h_client_architecture_support(PowerPCCPU *cpu,
> > >       /* For the future use: here @ov_table points to the first option 
> > > vector */
> > >       ov_table = addr;
> > > +    if (!ov_table) {
> > > +        return H_PARAMETER;
> > > +    }
> > 
> > This doesn't look right to check ov_table, I'd check addr directly instead:
> > 
> > -- >8 --
> > @@ -1679,12 +1679,16 @@ static target_ulong
> > h_client_architecture_support(PowerPCCPU *cpu,
> > 
> >      cas_pvr = cas_check_pvr(spapr, cpu, &addr, &raw_mode_supported,
> > &local_err);
> >      if (local_err) {
> >          error_report_err(local_err);
> >          return H_HARDWARE;
> >      }
> > +    if (!addr) {
> > +        // error_report*()
> > +        return H_PARAMETER;
> > +    }
> > 
> >      /* Update CPUs */
> >      if (cpu->compat_pvr != cas_pvr) {
> > ---
> > 
> > Still I'm not sure it makes sense, because the guest can also set other
> > invalid addresses such addr=0x69.
> 
> Neither is correct.  As you point out this filters at most one of many
> bad addresses.  And, in fact it's not even a bad address - there's no
> inherent reason the CAS information couldn't be put at guest address
> 0.
> 

Yes you're right, the guest can pass 0 as the address of the CAS structure.
But ov_table is the address of the vector table which comes after the PVR
list in the CAS structure, so it _cannot_ be zero. It is calculated in
cas_check_pvr() by incrementing the address passed by the guest while
parsing the PVR list. I was thinking that the guest could pass a value
that could cause addr to wrap and we end up with 0... but this cannot
happen actually since addr is a real address (60 bits) as returned by
ppc64_phys_to_real() and cas_check_pvr() can increment it no more than
512*8. Definitely not enough to wrap.

I'll simply drop this check. If the g_assert() in spapr_ovec_parse_vector()
is hit then it can only be the consequence of a bug in QEMU.

> 
> > 
> > >       ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
> > > +    if (!ov1_guest) {
> > > +        return H_PARAMETER;
> > > +    }
> > 
> > This one is OK (unlikely case where vector 1 isn't present).
> > 
> > >       ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
> > > +    if (!ov5_guest) {
> > > +        return H_PARAMETER;
> > > +    }
> > 
> > This one is OK too (unlikely case where vector 5 isn't present).
> > 
> > >       if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
> > >           error_report("guest requested hash and radix MMU, which is 
> > > invalid.");
> > >           exit(EXIT_FAILURE);
> > > 
> > > 
> > 
> 
> I agree these ones are ok, though.
>

pgpMxva6qY05U.pgp
Description: OpenPGP digital signature