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

[David Gibson]

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?

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

signature.asc
Description: PGP signature