| Message ID | 20231109171525.1129353-1-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ppc/pnv: Fix potential overflow in I2C model | expand |
On Thu, 2023-11-09 at 18:15 +0100, Cédric Le Goater wrote: > Coverity warns that "i2c_bus_busy(i2c->busses[i]) << i" might > overflow > because the expression is evaluated using 32-bit arithmetic and then > used in a context expecting a uint64_t. > > While we are at it, introduce a PNV_I2C_MAX_BUSSES constant and check > the number of busses at realize time. > > Fixes: Coverity CID 1523918 > Cc: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > v2: - check PNV_I2C_MAX_BUSSES in realize > - removed extra space before cast. > > hw/ppc/pnv_i2c.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > index f75e59e70977..483d91d15a77 100644 > --- a/hw/ppc/pnv_i2c.c > +++ b/hw/ppc/pnv_i2c.c > @@ -151,6 +151,7 @@ > #define I2C_RESET_S_SDA_REG 0x11 > > #define PNV_I2C_FIFO_SIZE 8 > +#define PNV_I2C_MAX_BUSSES 64 > > static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c) > { > @@ -437,7 +438,7 @@ static uint64_t pnv_i2c_xscom_read(void *opaque, > hwaddr addr, > case I2C_PORT_BUSY_REG: /* compute busy bit for each port */ > val = 0; > for (i = 0; i < i2c->num_busses; i++) { > - val |= i2c_bus_busy(i2c->busses[i]) << i; > + val |= (uint64_t)i2c_bus_busy(i2c->busses[i]) << i; > } > break; > > @@ -641,6 +642,11 @@ static void pnv_i2c_realize(DeviceState *dev, > Error **errp) > > assert(i2c->chip); > > + if (i2c->num_busses > PNV_I2C_MAX_BUSSES) { > + error_setg(errp, "Invalid number of busses: %u", i2c- > >num_busses); > + return; > + } > + > pnv_xscom_region_init(&i2c->xscom_regs, OBJECT(i2c), > &pnv_i2c_xscom_ops, > i2c, "xscom-i2c", PNV9_XSCOM_I2CM_SIZE); > Thanks, Cédric. Looks good to me! Reviewed-by: Glenn Miles <milesg@linux.vnet.ibm.com>
On 9/11/23 18:15, Cédric Le Goater wrote: > Coverity warns that "i2c_bus_busy(i2c->busses[i]) << i" might overflow > because the expression is evaluated using 32-bit arithmetic and then > used in a context expecting a uint64_t. > > While we are at it, introduce a PNV_I2C_MAX_BUSSES constant and check > the number of busses at realize time. > > Fixes: Coverity CID 1523918 > Cc: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > v2: - check PNV_I2C_MAX_BUSSES in realize > - removed extra space before cast. > > hw/ppc/pnv_i2c.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c index f75e59e70977..483d91d15a77 100644 --- a/hw/ppc/pnv_i2c.c +++ b/hw/ppc/pnv_i2c.c @@ -151,6 +151,7 @@ #define I2C_RESET_S_SDA_REG 0x11 #define PNV_I2C_FIFO_SIZE 8 +#define PNV_I2C_MAX_BUSSES 64 static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c) { @@ -437,7 +438,7 @@ static uint64_t pnv_i2c_xscom_read(void *opaque, hwaddr addr, case I2C_PORT_BUSY_REG: /* compute busy bit for each port */ val = 0; for (i = 0; i < i2c->num_busses; i++) { - val |= i2c_bus_busy(i2c->busses[i]) << i; + val |= (uint64_t)i2c_bus_busy(i2c->busses[i]) << i; } break; @@ -641,6 +642,11 @@ static void pnv_i2c_realize(DeviceState *dev, Error **errp) assert(i2c->chip); + if (i2c->num_busses > PNV_I2C_MAX_BUSSES) { + error_setg(errp, "Invalid number of busses: %u", i2c->num_busses); + return; + } + pnv_xscom_region_init(&i2c->xscom_regs, OBJECT(i2c), &pnv_i2c_xscom_ops, i2c, "xscom-i2c", PNV9_XSCOM_I2CM_SIZE);
Coverity warns that "i2c_bus_busy(i2c->busses[i]) << i" might overflow because the expression is evaluated using 32-bit arithmetic and then used in a context expecting a uint64_t. While we are at it, introduce a PNV_I2C_MAX_BUSSES constant and check the number of busses at realize time. Fixes: Coverity CID 1523918 Cc: Glenn Miles <milesg@linux.vnet.ibm.com> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- v2: - check PNV_I2C_MAX_BUSSES in realize - removed extra space before cast. hw/ppc/pnv_i2c.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)