[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH] spapr: Fail CAS if option vector table cannot be parsed
Date: Thu, 16 Jan 2020 19:29:02 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 1/16/20 5:13 PM, Greg Kurz wrote:
On Thu, 16 Jan 2020 16:34:06 +0100
Philippe Mathieu-Daudé <address@hidden> 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 <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:

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,
       if (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é <address@hidden>

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]