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 <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
> 
> 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) {
>