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
>