Skip to contentSkip to conversation

ppc/pnv: check size before data buffer access

PATCH Applied P J P 4 participants 5 messages Oct 26, 2018 – Nov 8, 2018

lore
Share & tools
lore

P J P

author
#0 lore raw

PATCH v2 1/1 ppc/pnv: check size before data buffer access

From: Prasad J Pandit <[email protected]>

While performing PowerNV memory r/w operations, the access length 'sz' could exceed the data[4] buffer size. Add check to avoid OOB access.

Reported-by Moguofang <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/ppc/pnv_lpc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Update v2: add error log message
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html

hw/ppc/pnv_lpc.c +7 -1
index d7721320a2..172a915cfc 100644
@@ -155,9 +155,15 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     /* XXX Check for magic bits at the top, addr size etc... */
     unsigned int sz = (cmd & ECCB_CTL_SZ_MASK) >> ECCB_CTL_SZ_LSH;
     uint32_t opb_addr = cmd & ECCB_CTL_ADDR_MASK;
-    uint8_t data[4];
+    uint8_t data[8];
     bool success;
 
+    if (sz > sizeof(data)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+            "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz);
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {
Signature · 1 line
2.17.2
Reviewed-by on v2 1/1
#1 lore raw
Quoted 8 lines · P J P

From: Prasad J Pandit <[email protected]>

While performing PowerNV memory r/w operations, the access length 'sz' could exceed the data[4] buffer size. Add check to avoid OOB access.

Reported-by Moguofang <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
Reviewed-by Cédric Le Goater <[email protected]>

Thanks,

C.

Quoted 29 lines
---
 hw/ppc/pnv_lpc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Update v2: add error log message
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html

hw/ppc/pnv_lpc.c +7 -1
index d7721320a2..172a915cfc 100644
@@ -155,9 +155,15 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     /* XXX Check for magic bits at the top, addr size etc... */
     unsigned int sz = (cmd & ECCB_CTL_SZ_MASK) >> ECCB_CTL_SZ_LSH;
     uint32_t opb_addr = cmd & ECCB_CTL_ADDR_MASK;
-    uint8_t data[4];
+    uint8_t data[8];
     bool success;
 
+    if (sz > sizeof(data)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+            "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz);
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {
Applied
#2 lore raw
In reply to P J P
Quoted 8 lines · P J P

From: Prasad J Pandit <[email protected]>

While performing PowerNV memory r/w operations, the access length 'sz' could exceed the data[4] buffer size. Add check to avoid OOB access.

Reported-by Moguofang <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>

Applied to ppc-for-3.1, thanks.

Quoted 28 lines
---
 hw/ppc/pnv_lpc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Update v2: add error log message
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html

hw/ppc/pnv_lpc.c +7 -1
index d7721320a2..172a915cfc 100644
@@ -155,9 +155,15 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     /* XXX Check for magic bits at the top, addr size etc... */
     unsigned int sz = (cmd & ECCB_CTL_SZ_MASK) >> ECCB_CTL_SZ_LSH;
     uint32_t opb_addr = cmd & ECCB_CTL_ADDR_MASK;
-    uint8_t data[4];
+    uint8_t data[8];
     bool success;
 
+    if (sz > sizeof(data)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+            "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz);
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {
Signature · 4 lines
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
#3 lore raw
In reply to P J P
Quoted 20 lines · P J P

From: Prasad J Pandit <[email protected]>

While performing PowerNV memory r/w operations, the access length 'sz' could exceed the data[4] buffer size. Add check to avoid OOB access.

Reported-by Moguofang <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/ppc/pnv_lpc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Update v2: add error log message
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html

hw/ppc/pnv_lpc.c +0 -0
index d7721320a2..172a915cfc 100644

hw/ppc/pnv_lpc.c static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd) · line 159

Quoted 6 lines
hw/ppc/pnv_lpc.c +1 -1
@@ -155,9 +155,15 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     /* XXX Check for magic bits at the top, addr size etc... */
     unsigned int sz = (cmd & ECCB_CTL_SZ_MASK) >> ECCB_CTL_SZ_LSH;
     uint32_t opb_addr = cmd & ECCB_CTL_ADDR_MASK;
-    uint8_t data[4];
+    uint8_t data[8];
     bool success;

I'm not sure 8 is enough.

ECCB_CTL_SZ_MASK is PPC_BITMASK(4, 7), and ECCB_CTL_SZ_LSH is (63 - 7). So the bitmask is 4 bits wide, and thus sz can be 0 to 15.

I think data[] size should be 16.

Thanks,
Laurent

#4 lore raw

Hello Laurent,

Quoted 30 lines · Laurent Vivier
Quoted 20 lines · P J P

From: Prasad J Pandit <[email protected]>

While performing PowerNV memory r/w operations, the access length 'sz' could exceed the data[4] buffer size. Add check to avoid OOB access.

Reported-by Moguofang <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/ppc/pnv_lpc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Update v2: add error log message
-> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html

hw/ppc/pnv_lpc.c +0 -0
index d7721320a2..172a915cfc 100644

hw/ppc/pnv_lpc.c static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd) · line 159

Quoted 6 lines
hw/ppc/pnv_lpc.c +1 -1
@@ -155,9 +155,15 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     /* XXX Check for magic bits at the top, addr size etc... */
     unsigned int sz = (cmd & ECCB_CTL_SZ_MASK) >> ECCB_CTL_SZ_LSH;
     uint32_t opb_addr = cmd & ECCB_CTL_ADDR_MASK;
-    uint8_t data[4];
+    uint8_t data[8];
     bool success;

I'm not sure 8 is enough.

Quoted 3 lines

ECCB_CTL_SZ_MASK is PPC_BITMASK(4, 7), and ECCB_CTL_SZ_LSH is (63 - 7). So the bitmask is 4 bits wide, and thus sz can be 0 to 15.
I think data[] size should be 16.

The bitmask allows more that 8 but the specs says that 1, 2, 4, 8 are the possible value size. So We should be fine.

C.

Quoted 4 lines

Thanks,
Laurent