| Message ID | 20181022121907.13635-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] arm: check bit index before use | expand |
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) {
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) { >
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
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 >
+-- 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
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) {