On Wed, Jan 13, 2021 at 8:03 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 12 Jan 2021 at 16:58, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > From: Hao Wu <wuhaotsh@google.com>
> >
> > The PWM module is part of NPCM7XX module. Each NPCM7XX module has two
> > identical PWM modules. Each module contains 4 PWM entries. Each PWM has
> > two outputs: frequency and duty_cycle. Both are computed using inputs
> > from software side.
>
> Hi; Coverity reports a possibly-overflowing arithmetic operation here
> (CID 1442342):
>
> > +static uint32_t npcm7xx_pwm_calculate_duty(NPCM7xxPWM *p)
> > +{
> > +    uint64_t duty;
> > +
> > +    if (p->running) {
> > +        if (p->cnr == 0) {
> > +            duty = 0;
> > +        } else if (p->cmr >= p->cnr) {
> > +            duty = NPCM7XX_PWM_MAX_DUTY;
> > +        } else {
> > +            duty = NPCM7XX_PWM_MAX_DUTY * (p->cmr + 1) / (p->cnr + 1);
>
> Here all of p->cmr, p->cnr and NPCM7XX_PWM_MAX_DUTY are 32-bits,
> so we calculate the whole expression using 32-bit arithmetic
> before assigning it to a 64-bit variable. This could be
> fixed using eg a cast of NPCM7XX_PWM_MAX_DUTY to uint64_t.
>
> Incidentally, we don't actually do any 64-bit
> arithmetic calculations on 'duty' and we return
> a uint32_t from this function, so 'duty' itself could
> be a uint32_t, I think...
>
Since NPCM7XX_PWM_MAX_DUTY =1,000,000 and p->cmr can have up to 65535, The
overflow is possible. We might want to cast NPCM7XX_PWM_MAX_DUTY to
uint64_t or #define NPCM7XX_PWM_MAX_DUTY 1000000ULL
duty itself could be a uint32_t as you point out. Since p->cmr is less than
p->cnr in this line, duty cannot exceed NPCM7XX_PWM_MAX_DUTY, so there's no
overflow after this computation.

Thank you for finding this!

Hao

>
> > +        }
> > +    } else {
> > +        duty = 0;
> > +    }
> > +
> > +    if (p->inverted) {
> > +        duty = NPCM7XX_PWM_MAX_DUTY - duty;
> > +    }
> > +
> > +    return duty;
> > +}
>
> thanks
> -- PMM
>