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

[Peter Maydell]

On Wed, 13 Jan 2021 at 17:13, Hao Wu <wuhaotsh@google.com> wrote:
> On Wed, Jan 13, 2021 at 8:03 AM Peter Maydell <peter.maydell@linaro.org> 
> wrote:
>> 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.

Hi; were you planning to write a patch for this?

thanks
-- PMM