| Message ID | 20231109080536.1005500-1-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | ppc/pnv: Fix potential overflow in I2C model | expand |
On 9/11/23 09:05, 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. > > Fixes: Coverity CID 1523918 > Cc: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv_i2c.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > index f75e59e70977..ab73c59f7704 100644 > --- a/hw/ppc/pnv_i2c.c > +++ b/hw/ppc/pnv_i2c.c > @@ -437,7 +437,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; Alternatively: val = deposit64(val, i, 1, i2c_bus_busy(i2c->busses[i])); > } > break; >
On Thu, 9 Nov 2023 at 08:06, Cédric Le Goater <clg@kaod.org> 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. > > Fixes: Coverity CID 1523918 > Cc: Glenn Miles <milesg@linux.vnet.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv_i2c.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > index f75e59e70977..ab73c59f7704 100644 > --- a/hw/ppc/pnv_i2c.c > +++ b/hw/ppc/pnv_i2c.c > @@ -437,7 +437,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; Should the device's realize function also impose a max limit on the num-busses property? There doesn't seem to be anything preventing a caller from setting it to a big number like 128, which would then be UB here. Style nit: casts shouldn't have a space after them before the thing they're casting. thanks -- PMM
On 11/9/23 16:02, Peter Maydell wrote: > On Thu, 9 Nov 2023 at 08:06, Cédric Le Goater <clg@kaod.org> 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. >> >> Fixes: Coverity CID 1523918 >> Cc: Glenn Miles <milesg@linux.vnet.ibm.com> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/ppc/pnv_i2c.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c >> index f75e59e70977..ab73c59f7704 100644 >> --- a/hw/ppc/pnv_i2c.c >> +++ b/hw/ppc/pnv_i2c.c >> @@ -437,7 +437,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; > > Should the device's realize function also impose a max > limit on the num-busses property? There doesn't seem to be > anything preventing a caller from setting it to a big > number like 128, which would then be UB here. yes. I will add an assert(i2c->num_busses < 64). The current max is 16 for POWER10. > Style nit: casts shouldn't have a space after them before > the thing they're casting. yep. I prefer the cast method than the deposit call. Philippe, I hope you don't mind ? Thanks, C.
On Thu, 9 Nov 2023 at 15:54, Cédric Le Goater <clg@kaod.org> wrote: > > On 11/9/23 16:02, Peter Maydell wrote: > > On Thu, 9 Nov 2023 at 08:06, Cédric Le Goater <clg@kaod.org> 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. > >> > >> Fixes: Coverity CID 1523918 > >> Cc: Glenn Miles <milesg@linux.vnet.ibm.com> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/ppc/pnv_i2c.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c > >> index f75e59e70977..ab73c59f7704 100644 > >> --- a/hw/ppc/pnv_i2c.c > >> +++ b/hw/ppc/pnv_i2c.c > >> @@ -437,7 +437,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; > > > > Should the device's realize function also impose a max > > limit on the num-busses property? There doesn't seem to be > > anything preventing a caller from setting it to a big > > number like 128, which would then be UB here. > > yes. I will add an assert(i2c->num_busses < 64). The current max > is 16 for POWER10. We generally make that kind of "property out of range" check be an error_setg()-and-return, rather than an assert. thanks -- PMM
On 9/11/23 16:54, Cédric Le Goater wrote: > On 11/9/23 16:02, Peter Maydell wrote: >> On Thu, 9 Nov 2023 at 08:06, Cédric Le Goater <clg@kaod.org> 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. >>> >>> Fixes: Coverity CID 1523918 >>> Cc: Glenn Miles <milesg@linux.vnet.ibm.com> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/ppc/pnv_i2c.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c >>> index f75e59e70977..ab73c59f7704 100644 >>> --- a/hw/ppc/pnv_i2c.c >>> +++ b/hw/ppc/pnv_i2c.c >>> @@ -437,7 +437,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; >> >> Should the device's realize function also impose a max >> limit on the num-busses property? There doesn't seem to be >> anything preventing a caller from setting it to a big >> number like 128, which would then be UB here. > > yes. I will add an assert(i2c->num_busses < 64). The current max > is 16 for POWER10. > >> Style nit: casts shouldn't have a space after them before >> the thing they're casting. > > yep. > > I prefer the cast method than the deposit call. Philippe, I hope you > don't mind ? Matter of taste, I don't mind ¯\_(ツ)_/¯
diff --git a/hw/ppc/pnv_i2c.c b/hw/ppc/pnv_i2c.c index f75e59e70977..ab73c59f7704 100644 --- a/hw/ppc/pnv_i2c.c +++ b/hw/ppc/pnv_i2c.c @@ -437,7 +437,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;
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. Fixes: Coverity CID 1523918 Cc: Glenn Miles <milesg@linux.vnet.ibm.com> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/ppc/pnv_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)