| Message ID | 157918715618.376249.7891210201270364781.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Fail CAS if option vector table cannot be parsed | expand |
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); > >
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); > > > > >
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); >>> >>> >> >
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.
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. >
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);
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(+)