[Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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 <moguofang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/ppc/pnv_lpc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index d7721320a2..f5e5bd4053 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -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) {
-- 
2.17.2

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Mon, Oct 22, 2018 at 05:49:07PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> 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 <moguofang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

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?

> ---
>  hw/ppc/pnv_lpc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index d7721320a2..f5e5bd4053 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -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) {

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[Cédric Le Goater]

On 10/23/18 5:37 PM, David Gibson wrote:
> On Mon, Oct 22, 2018 at 05:49:07PM +0530, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> 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 <moguofang@huawei.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> 
> 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?

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.

> 
>> ---
>>  hw/ppc/pnv_lpc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
>> index d7721320a2..f5e5bd4053 100644
>> --- a/hw/ppc/pnv_lpc.c
>> +++ b/hw/ppc/pnv_lpc.c
>> @@ -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) {
> 

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[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.

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?

===
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index d7721320a2..655d5f3d07 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -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

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[Cédric Le Goater]

Hello Prasad,

On 10/25/18 8:45 AM, P J P wrote:
>   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? 

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.

> If yes, kindly include me in CC please.

yes sure.  

> 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.

> ===
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index d7721320a2..655d5f3d07 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -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
> 

Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access

[P J P]

+-- 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