Skip to contentSkip to conversation

ppc/pnv: check size before data buffer access

PATCH In review P J P 3 participants 6 messages Oct 22, 2018 – Oct 26, 2018

lore
Share & tools
lore

P J P

author
#0 lore raw

PATCH 3/3 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 | 4 ++++
 1 file changed, 4 insertions(+)
hw/ppc/pnv_lpc.c +4 -0
index d7721320a2..f5e5bd4053 100644
@@ -158,6 +158,10 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     uint8_t data[4];
     bool success;
 
+    if (sz > sizeof(data)) {
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {
Signature · 1 line
2.17.2
#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]>

So, it certainly does look like we can get an overrun here. But is just turning the access into a no-op if the size is too large the correct behaviour? It doesn't seem a very likely behaviour for the actual hardware.

Should we be reporting an error via some register bits? Or should we be masking or truncating the size field instead the size down to something smaller?

BenH or Cedric, do you know how the hardware actually behaves here?

Quoted 19 lines
---
 hw/ppc/pnv_lpc.c | 4 ++++
 1 file changed, 4 insertions(+)
hw/ppc/pnv_lpc.c +4 -0
index d7721320a2..f5e5bd4053 100644
@@ -158,6 +158,10 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     uint8_t data[4];
     bool success;
 
+    if (sz > sizeof(data)) {
+        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
#2 lore raw
Quoted 15 lines · David Gibson
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]>

So, it certainly does look like we can get an overrun here. But is just turning the access into a no-op if the size is too large the correct behaviour? It doesn't seem a very likely behaviour for the actual hardware.

Quoted 5 lines

Should we be reporting an error via some register bits? Or should we be masking or truncating the size field instead the size down to something smaller?

BenH or Cedric, do you know how the hardware actually behaves here?

8bytes reads and writes are supported by the ECCB, which interfaces with the OPB master, which interfaces with the LPC HC. The HW is bit complex in that area. There are a few legacy devices.

skiboot only uses 1 and 4 bytes accesses if I am correct so we didn't fall into the trap.

I think using a data[8] would be more appropriate. It would make the pnv_lpc_do_eccb() routine a little more complex. I tried to rewrite it to have a common one with the P9 LPC model but could not find a common pattern. P9 is purely MMIO based. Something on the TODO list.

8 bytes accesses will then fail anyhow because all MemoryRegionOps have a max_access_size = 4.

Thanks,

C.

Quoted 21 lines
Quoted 20 lines
---
 hw/ppc/pnv_lpc.c | 4 ++++
 1 file changed, 4 insertions(+)
hw/ppc/pnv_lpc.c +4 -0
index d7721320a2..f5e5bd4053 100644
@@ -158,6 +158,10 @@ static void pnv_lpc_do_eccb(PnvLpcController *lpc, uint64_t cmd)
     uint8_t data[4];
     bool success;
 
+    if (sz > sizeof(data)) {
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {
author
#3 lore raw

Hello Cedric,

+-- On Wed, 24 Oct 2018, Cédric Le Goater wrote --+
| I think using a data[8] would be more appropriate. It would make the 
| pnv_lpc_do_eccb() routine a little more complex. I tried to rewrite it to 
| have a common one with the P9 LPC model but could not find a common pattern. 
| P9 is purely MMIO based. Something on the TODO list.
| 
| 8 bytes accesses will then fail anyhow because all MemoryRegionOps have a 
| max_access_size = 4.

Thank you for these inputs, appreciate it. To confirm,

- You plan to send a v2 patch to fix the OOB access issue along with refactoring pnv_lpc_do_eccb? If yes, kindly include me in CC please.

OR

- While we refactor the routine for better, a patch below seem okay to fix the OOB access issue?

===

hw/ppc/pnv_lpc.c +0 -0
index d7721320a2..655d5f3d07 100644
@@ -155,9 +155,13 @@ 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)) {
+ return;
+ }
+

     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);
         if (success) {

===

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

#4 lore raw

Hello Prasad,

Quoted 11 lines · P J P

Hello Cedric,

+-- On Wed, 24 Oct 2018, Cédric Le Goater wrote --+
| I think using a data[8] would be more appropriate. It would make the 
| pnv_lpc_do_eccb() routine a little more complex. I tried to rewrite it to 
| have a common one with the P9 LPC model but could not find a common pattern. 
| P9 is purely MMIO based. Something on the TODO list.
| 
| 8 bytes accesses will then fail anyhow because all MemoryRegionOps have a 
| max_access_size = 4.
Quoted 4 lines

Thank you for these inputs, appreciate it. To confirm,

- You plan to send a v2 patch to fix the OOB access issue along with refactoring pnv_lpc_do_eccb?

we might improve pnv_lpc_do_eccb() when the P9 LPC model is sent. I am not sure of that as this is for P8 only.

Quoted 1 line

If yes, kindly include me in CC please.

yes sure.

Quoted 4 lines

OR

- While we refactor the routine for better, a patch below seem okay to fix the OOB access issue?

I think it is fine. Please add something like :

	qemu_log_mask(LOG_GUEST_ERROR, "ECCB: invalid operation at @0x%08x" 
			" size %d\n", opb_addr, sz);

Thanks,

C.

Quoted 29 lines

===

hw/ppc/pnv_lpc.c +5 -1
index d7721320a2..655d5f3d07 100644
@@ -155,9 +155,13 @@ 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)) {
+        return;
+    }
+
     if (cmd & ECCB_CTL_READ) {
         success = opb_read(lpc, opb_addr, data, sz);

         if (success) {

===

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

author
#5 lore raw
+-- On Fri, 26 Oct 2018, Cédric Le Goater wrote --+
| On 10/25/18 8:45 AM, P J P wrote:
| >  - While we refactor the routine for better, a patch below seem okay to fix 
| >    the OOB access issue?
| 
| I think it is fine. Please add something like :  
| 
| 	qemu_log_mask(LOG_GUEST_ERROR, "ECCB: invalid operation at @0x%08x" 
| 			" size %d\n", opb_addr, sz);

Okay, will do.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F