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

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

Posted by Greg Kurz 4 years, 3 months ago

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;
+    }
 
     ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
+    if (!ov1_guest) {
+        return H_PARAMETER;
+    }
     ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
+    if (!ov5_guest) {
+        return H_PARAMETER;
+    }
     if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
         error_report("guest requested hash and radix MMU, which is invalid.");
         exit(EXIT_FAILURE);

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

Posted by Philippe Mathieu-Daudé 4 years, 3 months ago

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.

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

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

Posted by David Gibson 4 years, 3 months ago

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

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

Posted by Greg Kurz 4 years, 3 months ago

On Fri, 17 Jan 2020 15:46:57 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <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.
> 

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

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

Posted by Greg Kurz 4 years, 3 months ago

On Thu, 16 Jan 2020 16:34:06 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Greg,
> 

Hi Phil,

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

I decided to check ov_table because this is what we pass to
spapr_ovec_parse_vector() and that shouldn't be NULL.

> -- >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;
> +    }
> 

I don't really care one way or another, but adding an error_report() is a
good idea since linux just print out the following in case of CAS failure:

WARNING: ibm,client-architecture-support call FAILED!

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

The point of this patch is just to avoid hitting the assertions. 0x69
is probably bullshit but it passes the g_assert() at least.

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

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

Posted by Philippe Mathieu-Daudé 4 years, 3 months ago

On 1/16/20 5:13 PM, Greg Kurz wrote:
> On Thu, 16 Jan 2020 16:34:06 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Hi Greg,
>>
> 
> Hi Phil,
> 
>> 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:
>>
> 
> I decided to check ov_table because this is what we pass to
> spapr_ovec_parse_vector() and that shouldn't be NULL.

OK, it makes sense.

>> -- >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;
>> +    }
>>
> 
> I don't really care one way or another, but adding an error_report() is a
> good idea since linux just print out the following in case of CAS failure:
> 
> WARNING: ibm,client-architecture-support call FAILED!

With some error_report:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>>        /* 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.
>>
> 
> The point of this patch is just to avoid hitting the assertions. 0x69
> is probably bullshit but it passes the g_assert() at least.
> 
>>>    
>>>        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);
>>>
>>>
>>
>