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 >