* [PATCH] spapr: Fail CAS if option vector table cannot be parsed
@ 2020-01-16 15:05 Greg Kurz
2020-01-16 15:34 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2020-01-16 15:05 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
2020-01-16 15:05 [PATCH] spapr: Fail CAS if option vector table cannot be parsed Greg Kurz
@ 2020-01-16 15:34 ` Philippe Mathieu-Daudé
2020-01-16 16:13 ` Greg Kurz
2020-01-17 5:46 ` David Gibson
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-16 15:34 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel
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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
2020-01-16 15:34 ` Philippe Mathieu-Daudé
@ 2020-01-16 16:13 ` Greg Kurz
2020-01-16 18:29 ` Philippe Mathieu-Daudé
2020-01-17 5:46 ` David Gibson
1 sibling, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2020-01-16 16:13 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-ppc, qemu-devel, David Gibson
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);
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
2020-01-16 16:13 ` Greg Kurz
@ 2020-01-16 18:29 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-16 18:29 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, David Gibson
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);
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
2020-01-16 15:34 ` Philippe Mathieu-Daudé
2020-01-16 16:13 ` Greg Kurz
@ 2020-01-17 5:46 ` David Gibson
2020-01-17 9:10 ` Greg Kurz
1 sibling, 1 reply; 6+ messages in thread
From: David Gibson @ 2020-01-17 5:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-ppc, Greg Kurz, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
2020-01-17 5:46 ` David Gibson
@ 2020-01-17 9:10 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2020-01-17 9:10 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]
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.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-17 9:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 15:05 [PATCH] spapr: Fail CAS if option vector table cannot be parsed Greg Kurz
2020-01-16 15:34 ` Philippe Mathieu-Daudé
2020-01-16 16:13 ` Greg Kurz
2020-01-16 18:29 ` Philippe Mathieu-Daudé
2020-01-17 5:46 ` David Gibson
2020-01-17 9:10 ` Greg Kurz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).