Re: [PATCH 8/8] platform: vivid-cec: fix potential integer overflow in vivid_cec_pin_adap_events

[Gustavo A. R. Silva]

Quoting Hans Verkuil <hverkuil@xxxxxxxxx>:

[...]

> > > What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL +
> > >
> > > I think that forces everything else in the expression to be evaluated
> > > as u64.
> >
> > Well, in this case the operator precedence takes place and the
> > expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the
> > issue remains the same.
> >
> > I can switch the expressions as follows:
> >
> > (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL
>
> What about:
>
> 10ULL * len * ...

Yeah, I like it.

> > and avoid the cast in the middle.
> >
> > What do you think?
>
> My problem is that (u64)len suggests that there is some problem with len
> specifically, which isn't true.

That's a good point. Actually, I think the same applies for the rest  
of the patch series. Maybe it is a good idea to send a v2 of the whole  
patchset with that update.

> > > It definitely needs a comment that this fixes a bogus Coverity report.
> >
> > I actually added the following line to the message changelog:
> > Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
>
> That needs to be in the source, otherwise someone will remove the
> cast (or ULL) at some time in the future since it isn't clear why
> it is done. And nobody reads commit logs from X years back :-)

You're right. I thought you were talking about the changelog.

And unless you think otherwise, I think there is no need for any  
additional code comment if the update you suggest is applied:

len * 10ULL * CEC_TIM_DATA_BIT_TOTAL

Thanks
--
Gustavo