[Qemu-devel] [RFC PATCH v0 0/2] target-ppc: Fix an invalid free

[Qemu-devel] [RFC PATCH v0 0/2] target-ppc: Fix an invalid free

[Bharata B Rao]

g_free() in target-ppc/translate_init.c:ppc_cpu_unrealizefn() can fail
due to invalid pointer being passed to it. Fix this along with a cleanup.

I have never seen ppc_cpu_unrealizefn() getting called for sPAPR guests,
but I ran into this issue when I was adding unrealize call from the CPU
hot removal path while working on CPU [un]hotplug support for sPAPR guests.

Bharata B Rao (2):
  target-ppc: Use macros in opcodes table handling code
  target-ppc: Fix an invalid free in opcode table handling code.

 target-ppc/cpu.h            |  3 ++-
 target-ppc/translate_init.c | 43 ++++++++++++++++++++++++++++++-------------
 2 files changed, 32 insertions(+), 14 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v0 1/2] target-ppc: Use macros in opcodes table handling code

[Bharata B Rao]

Define and use macros instead of direct numbers wherever
possible in ppc opcodes table handling code.

This doesn't change any code functionality.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 target-ppc/cpu.h            |  3 ++-
 target-ppc/translate_init.c | 24 ++++++++++++++----------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index b64c652..981c6c8 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -924,7 +924,8 @@ struct ppc_segment_page_sizes {
 /* The whole PowerPC CPU context */
 #define NB_MMU_MODES 3
 
-#define PPC_CPU_OPCODES_LEN 0x40
+#define PPC_CPU_OPCODES_LEN          0x40
+#define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
 
 struct CPUPPCState {
     /* First are the most commonly used resources
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 48177ed..358a2a7 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8432,14 +8432,16 @@ enum {
     PPC_INDIRECT = 1, /* Indirect opcode table */
 };
 
+#define PPC_OPCODE_MASK 0x3
+
 static inline int is_indirect_opcode (void *handler)
 {
-    return ((uintptr_t)handler & 0x03) == PPC_INDIRECT;
+    return ((uintptr_t)handler & PPC_OPCODE_MASK) == PPC_INDIRECT;
 }
 
 static inline opc_handler_t **ind_table(void *handler)
 {
-    return (opc_handler_t **)((uintptr_t)handler & ~3);
+    return (opc_handler_t **)((uintptr_t)handler & ~PPC_OPCODE_MASK);
 }
 
 /* Instruction table creation */
@@ -8456,8 +8458,8 @@ static int create_new_table (opc_handler_t **table, unsigned char idx)
 {
     opc_handler_t **tmp;
 
-    tmp = g_new(opc_handler_t *, 0x20);
-    fill_new_table(tmp, 0x20);
+    tmp = g_new(opc_handler_t *, PPC_CPU_INDIRECT_OPCODES_LEN);
+    fill_new_table(tmp, PPC_CPU_INDIRECT_OPCODES_LEN);
     table[idx] = (opc_handler_t *)((uintptr_t)tmp | PPC_INDIRECT);
 
     return 0;
@@ -8584,7 +8586,8 @@ static int test_opcode_table (opc_handler_t **table, int len)
             table[i] = &invalid_handler;
         if (table[i] != &invalid_handler) {
             if (is_indirect_opcode(table[i])) {
-                tmp = test_opcode_table(ind_table(table[i]), 0x20);
+                tmp = test_opcode_table(ind_table(table[i]),
+                    PPC_CPU_INDIRECT_OPCODES_LEN);
                 if (tmp == 0) {
                     free(table[i]);
                     table[i] = &invalid_handler;
@@ -8602,7 +8605,7 @@ static int test_opcode_table (opc_handler_t **table, int len)
 
 static void fix_opcode_tables (opc_handler_t **ppc_opcodes)
 {
-    if (test_opcode_table(ppc_opcodes, 0x40) == 0)
+    if (test_opcode_table(ppc_opcodes, PPC_CPU_OPCODES_LEN) == 0)
         printf("*** WARNING: no opcode defined !\n");
 }
 
@@ -8613,7 +8616,7 @@ static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
     CPUPPCState *env = &cpu->env;
     opcode_t *opc;
 
-    fill_new_table(env->opcodes, 0x40);
+    fill_new_table(env->opcodes, PPC_CPU_OPCODES_LEN);
     for (opc = opcodes; opc < &opcodes[ARRAY_SIZE(opcodes)]; opc++) {
         if (((opc->handler.type & pcc->insns_flags) != 0) ||
             ((opc->handler.type2 & pcc->insns_flags2) != 0)) {
@@ -8639,12 +8642,12 @@ static void dump_ppc_insns (CPUPPCState *env)
 
     printf("Instructions set:\n");
     /* opc1 is 6 bits long */
-    for (opc1 = 0x00; opc1 < 0x40; opc1++) {
+    for (opc1 = 0x00; opc1 < PPC_CPU_OPCODES_LEN; opc1++) {
         table = env->opcodes;
         handler = table[opc1];
         if (is_indirect_opcode(handler)) {
             /* opc2 is 5 bits long */
-            for (opc2 = 0; opc2 < 0x20; opc2++) {
+            for (opc2 = 0; opc2 < PPC_CPU_INDIRECT_OPCODES_LEN; opc2++) {
                 table = env->opcodes;
                 handler = env->opcodes[opc1];
                 table = ind_table(handler);
@@ -8652,7 +8655,8 @@ static void dump_ppc_insns (CPUPPCState *env)
                 if (is_indirect_opcode(handler)) {
                     table = ind_table(handler);
                     /* opc3 is 5 bits long */
-                    for (opc3 = 0; opc3 < 0x20; opc3++) {
+                    for (opc3 = 0; opc3 < PPC_CPU_INDIRECT_OPCODES_LEN;
+                            opc3++) {
                         handler = table[opc3];
                         if (handler->handler != &gen_invalid) {
                             /* Special hack to properly dump SPE insns */
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v0 2/2] target-ppc: Fix an invalid free in opcode table handling code.

[Bharata B Rao]

Opcode table has direct, indirect and double indirect handlers, but
ppc_cpu_unrealizefn() frees direct handlers which are never allocated
and never frees double indirect handlers.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 target-ppc/translate_init.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 358a2a7..30f09b9 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9091,11 +9091,24 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(dev);
     CPUPPCState *env = &cpu->env;
-    int i;
+    opc_handler_t **table;
+    int i, j;
 
     for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
-        if (env->opcodes[i] != &invalid_handler) {
-            g_free(env->opcodes[i]);
+        if (env->opcodes[i] == &invalid_handler) {
+            continue;
+        }
+        if (is_indirect_opcode(env->opcodes[i])) {
+            table = ind_table(env->opcodes[i]);
+            for (j = 0; j < PPC_CPU_INDIRECT_OPCODES_LEN; j++) {
+                if (table[j] != &invalid_handler &&
+                        is_indirect_opcode(table[j])) {
+                    g_free((opc_handler_t *)((uintptr_t)table[j] &
+                        ~PPC_INDIRECT));
+                }
+            }
+            g_free((opc_handler_t *)((uintptr_t)env->opcodes[i] &
+                ~PPC_INDIRECT));
         }
     }
 }
-- 
1.7.11.7

Re: [Qemu-devel] [RFC PATCH v0 0/2] target-ppc: Fix an invalid free

[Alexander Graf]

On 26.09.14 11:07, Bharata B Rao wrote:
> g_free() in target-ppc/translate_init.c:ppc_cpu_unrealizefn() can fail
> due to invalid pointer being passed to it. Fix this along with a cleanup.
> 
> I have never seen ppc_cpu_unrealizefn() getting called for sPAPR guests,
> but I ran into this issue when I was adding unrealize call from the CPU
> hot removal path while working on CPU [un]hotplug support for sPAPR guests.

This opcode table handling code really is pretty ugly code :).

Thanks, applied both to ppc-next.


Alex