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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2760 bytes --]

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 <groug@kaod.org>
> > ---
> >   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.


> 
> >       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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]