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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9866dd3e-471e-b048-3f9c-21fab4f8b800@xs4all.nl>
Date:   Tue, 30 Jan 2018 12:50:33 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     "Gustavo A. R. Silva" <garsilva@...eddedor.com>
Cc:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 8/8] platform: vivid-cec: fix potential integer overflow
 in vivid_cec_pin_adap_events

On 01/30/18 12:43, Gustavo A. R. Silva wrote:
> 
> Quoting Hans Verkuil <hverkuil@...all.nl>:
> 
> [...]
> 
>>>> 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

I still think I'd like to see a comment. It is still not obvious why
you would want to use ULL here.

Please use "10ULL * len", it's actually a bit better to have it in
that order (it's 10 bits per character, so '10 * len' is a more logical
order).

Regards,

	Hans