* [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access @ 2018-10-22 12:19 P J P 2018-10-23 15:37 ` David Gibson 0 siblings, 1 reply; 6+ messages in thread From: P J P @ 2018-10-22 12:19 UTC (permalink / raw) To: Qemu Developers; +Cc: Alexander Graf, David Gibson, Moguofang, Prasad J Pandit 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access 2018-10-22 12:19 [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access P J P @ 2018-10-23 15:37 ` David Gibson 2018-10-24 17:03 ` Cédric Le Goater 0 siblings, 1 reply; 6+ messages in thread From: David Gibson @ 2018-10-23 15:37 UTC (permalink / raw) To: P J P Cc: Qemu Developers, Alexander Graf, Moguofang, Prasad J Pandit, Benjamin Herrenschmidt, clg [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access 2018-10-23 15:37 ` David Gibson @ 2018-10-24 17:03 ` Cédric Le Goater 2018-10-25 6:45 ` P J P 0 siblings, 1 reply; 6+ messages in thread From: Cédric Le Goater @ 2018-10-24 17:03 UTC (permalink / raw) To: David Gibson, P J P Cc: Qemu Developers, Alexander Graf, Moguofang, Prasad J Pandit, Benjamin Herrenschmidt 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) { > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access 2018-10-24 17:03 ` Cédric Le Goater @ 2018-10-25 6:45 ` P J P 2018-10-26 8:25 ` Cédric Le Goater 0 siblings, 1 reply; 6+ messages in thread From: P J P @ 2018-10-25 6:45 UTC (permalink / raw) To: Cédric Le Goater Cc: David Gibson, Qemu Developers, Alexander Graf, Moguofang, Benjamin Herrenschmidt 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access 2018-10-25 6:45 ` P J P @ 2018-10-26 8:25 ` Cédric Le Goater 2018-10-26 9:38 ` P J P 0 siblings, 1 reply; 6+ messages in thread From: Cédric Le Goater @ 2018-10-26 8:25 UTC (permalink / raw) To: P J P Cc: David Gibson, Qemu Developers, Alexander Graf, Moguofang, Benjamin Herrenschmidt 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access 2018-10-26 8:25 ` Cédric Le Goater @ 2018-10-26 9:38 ` P J P 0 siblings, 0 replies; 6+ messages in thread From: P J P @ 2018-10-26 9:38 UTC (permalink / raw) To: Cédric Le Goater Cc: David Gibson, Qemu Developers, Alexander Graf, Moguofang, Benjamin Herrenschmidt +-- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-26 9:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-22 12:19 [Qemu-devel] [PATCH 3/3] ppc/pnv: check size before data buffer access P J P 2018-10-23 15:37 ` David Gibson 2018-10-24 17:03 ` Cédric Le Goater 2018-10-25 6:45 ` P J P 2018-10-26 8:25 ` Cédric Le Goater 2018-10-26 9:38 ` P J P
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).