Re: [PULL 18/21] hw/misc: Add a PWM module for NPCM7XX

[Peter Maydell <peter.maydell@linaro.org>]

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...

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

thanks
-- PMM