| Message ID | 20181026123358.2554-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ppc/pnv: check size before data buffer access | expand |
On 10/26/18 2:33 PM, 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> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/ppc/pnv_lpc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > Update v2: add error log message > -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html > > diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c > index d7721320a2..172a915cfc 100644 > --- a/hw/ppc/pnv_lpc.c > +++ b/hw/ppc/pnv_lpc.c > @@ -155,9 +155,15 @@ 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)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz); > + return; > + } > + > if (cmd & ECCB_CTL_READ) { > success = opb_read(lpc, opb_addr, data, sz); > if (success) { >
On Fri, Oct 26, 2018 at 06:03:58PM +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> Applied to ppc-for-3.1, thanks. > --- > hw/ppc/pnv_lpc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > Update v2: add error log message > -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html > > diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c > index d7721320a2..172a915cfc 100644 > --- a/hw/ppc/pnv_lpc.c > +++ b/hw/ppc/pnv_lpc.c > @@ -155,9 +155,15 @@ 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)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz); > + return; > + } > + > if (cmd & ECCB_CTL_READ) { > success = opb_read(lpc, opb_addr, data, sz); > if (success) {
On 26/10/2018 14:33, 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> > --- > hw/ppc/pnv_lpc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > Update v2: add error log message > -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html > > diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c > index d7721320a2..172a915cfc 100644 > --- a/hw/ppc/pnv_lpc.c > +++ b/hw/ppc/pnv_lpc.c > @@ -155,9 +155,15 @@ 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; I'm not sure 8 is enough. ECCB_CTL_SZ_MASK is PPC_BITMASK(4, 7), and ECCB_CTL_SZ_LSH is (63 - 7). So the bitmask is 4 bits wide, and thus sz can be 0 to 15. I think data[] size should be 16. Thanks, Laurent
Hello Laurent, On 11/8/18 10:10 AM, Laurent Vivier wrote: > On 26/10/2018 14:33, 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> >> --- >> hw/ppc/pnv_lpc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> Update v2: add error log message >> -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05750.html >> >> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c >> index d7721320a2..172a915cfc 100644 >> --- a/hw/ppc/pnv_lpc.c >> +++ b/hw/ppc/pnv_lpc.c >> @@ -155,9 +155,15 @@ 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; > > I'm not sure 8 is enough. > > ECCB_CTL_SZ_MASK is PPC_BITMASK(4, 7), and ECCB_CTL_SZ_LSH is (63 - 7). > So the bitmask is 4 bits wide, and thus sz can be 0 to 15. > I think data[] size should be 16. The bitmask allows more that 8 but the specs says that 1, 2, 4, 8 are the possible value size. So We should be fine. C. > > Thanks, > Laurent >
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c index d7721320a2..172a915cfc 100644 --- a/hw/ppc/pnv_lpc.c +++ b/hw/ppc/pnv_lpc.c @@ -155,9 +155,15 @@ 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)) { + qemu_log_mask(LOG_GUEST_ERROR, + "ECCB: invalid operation at @0x%08x size %d\n", opb_addr, sz); + return; + } + if (cmd & ECCB_CTL_READ) { success = opb_read(lpc, opb_addr, data, sz); if (success) {