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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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