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

[Philippe Mathieu-Daudé]

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.

>   
>       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);