[PATCH 0/8] fix potential integer overflows

[PATCH 0/8] fix potential integer overflows

[Gustavo A. R. Silva]

This patchset aims to fix potential integer overflows reported
by Coverity.

In all cases the potential overflowing expressions are evaluated
using 32-bit arithmetic before being used in contexts that expect
a 64-bit arithmetic. So a cast to the proper type was added to each
of those expressions in order to avoid any potential integer overflow.

This also gives the compiler complete information about the proper
arithmetic for each expression and improves code quality.

Addresses the following Coverity IDs reported as
"Unintentional integer overflow" issues:

200604, 1056807, 1056808, 1271223,
1324146, 1392628, 1392630, 1446589,
1454996, 1458347.

Thank you

Gustavo A. R. Silva (8):
  rtl2832: fix potential integer overflow
  dvb-frontends: ves1820: fix potential integer overflow
  i2c: max2175: fix potential integer overflow in max2175_set_nco_freq
  i2c: ov9650: fix potential integer overflow in
    __ov965x_set_frame_interval
  pci: cx88-input: fix potential integer overflow
  rockchip/rga: fix potential integer overflow in rga_buf_map
  platform: sh_veu: fix potential integer overflow in
    sh_veu_colour_offset
  platform: vivid-cec: fix potential integer overflow in
    vivid_cec_pin_adap_events

 drivers/media/dvb-frontends/rtl2832.c         | 4 ++--
 drivers/media/dvb-frontends/ves1820.c         | 2 +-
 drivers/media/i2c/max2175.c                   | 2 +-
 drivers/media/i2c/ov9650.c                    | 2 +-
 drivers/media/pci/cx88/cx88-input.c           | 4 ++--
 drivers/media/platform/rockchip/rga/rga-buf.c | 2 +-
 drivers/media/platform/sh_veu.c               | 4 ++--
 drivers/media/platform/vivid/vivid-cec.c      | 2 +-
 8 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/8] rtl2832: fix potential integer overflow in rtl2832_set_frontend

[Gustavo A. R. Silva]

Cast dev->pdata->clk to u64 in order to avoid a potential integer
overflow. This variable is being used in a context that expects
an expression of type u64.

Addresses-Coverity-ID: 1271223 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/dvb-frontends/rtl2832.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
index 94bf5b7..eefc301 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -498,7 +498,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
 	* RSAMP_RATIO = floor(CrystalFreqHz * 7 * pow(2, 22)
 	*	/ ConstWithBandwidthMode)
 	*/
-	num = dev->pdata->clk * 7;
+	num = (u64)dev->pdata->clk * 7;
 	num *= 0x400000;
 	num = div_u64(num, bw_mode);
 	resamp_ratio =  num & 0x3ffffff;
@@ -511,7 +511,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
 	*	/ (CrystalFreqHz * 7))
 	*/
 	num = bw_mode << 20;
-	num2 = dev->pdata->clk * 7;
+	num2 = (u64)dev->pdata->clk * 7;
 	num = div_u64(num, num2);
 	num = -num;
 	cfreq_off_ratio = num & 0xfffff;
-- 
2.7.4

[PATCH 2/8] dvb-frontends: ves1820: fix potential integer overflow

[Gustavo A. R. Silva]

Cast state->config->xin to u64 in order to avoid a potential integer
overflow. This variable is being used in a context that expects an
expression of type u64.

Addresses-Coverity-ID: 200604 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/dvb-frontends/ves1820.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/ves1820.c b/drivers/media/dvb-frontends/ves1820.c
index 1d89792..9dbd582 100644
--- a/drivers/media/dvb-frontends/ves1820.c
+++ b/drivers/media/dvb-frontends/ves1820.c
@@ -137,7 +137,7 @@ static int ves1820_set_symbolrate(struct ves1820_state *state, u32 symbolrate)
 		NDEC = 3;
 
 	/* yeuch! */
-	fpxin = state->config->xin * 10;
+	fpxin = (u64)state->config->xin * 10;
 	fptmp = fpxin; do_div(fptmp, 123);
 	if (symbolrate < fptmp)
 		SFIL = 1;
-- 
2.7.4

[PATCH 3/8] i2c: max2175: fix potential integer overflow in max2175_set_nco_freq

[Gustavo A. R. Silva]

Cast expression (clock_rate - abs_nco_freq) to s64 in order to avoid
a potential integer overflow. This variable is being used in a
context that expects an expression of type s64.

Addresses-Coverity-ID: 1446589 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/i2c/max2175.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/max2175.c b/drivers/media/i2c/max2175.c
index 2f1966b..3401ae6 100644
--- a/drivers/media/i2c/max2175.c
+++ b/drivers/media/i2c/max2175.c
@@ -643,7 +643,7 @@ static int max2175_set_nco_freq(struct max2175 *ctx, s32 nco_freq)
 	if (abs_nco_freq < clock_rate / 2) {
 		nco_val_desired = 2 * nco_freq;
 	} else {
-		nco_val_desired = 2 * (clock_rate - abs_nco_freq);
+		nco_val_desired = 2 * (s64)(clock_rate - abs_nco_freq);
 		if (nco_freq < 0)
 			nco_val_desired = -nco_val_desired;
 	}
-- 
2.7.4

[PATCH 4/8] i2c: ov9650: fix potential integer overflow in __ov965x_set_frame_interval

[Gustavo A. R. Silva]

Cast fi->interval.numerator to u64 in order to avoid a potential integer
overflow. This variable is being used in a context that expects an
expression of type u64.

Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/i2c/ov9650.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index e519f27..c674a49 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1130,7 +1130,7 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
 	if (fi->interval.denominator == 0)
 		return -EINVAL;
 
-	req_int = (u64)(fi->interval.numerator * 10000) /
+	req_int = (u64)fi->interval.numerator * 10000 /
 		fi->interval.denominator;
 
 	for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
-- 
2.7.4

Re: [PATCH 4/8] i2c: ov9650: fix potential integer overflow in __ov965x_set_frame_interval

[Sakari Ailus]

On Mon, Jan 29, 2018 at 06:32:01PM -0600, Gustavo A. R. Silva wrote:
> Cast fi->interval.numerator to u64 in order to avoid a potential integer
> overflow. This variable is being used in a context that expects an
> expression of type u64.
> 
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/media/i2c/ov9650.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index e519f27..c674a49 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -1130,7 +1130,7 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
>  	if (fi->interval.denominator == 0)
>  		return -EINVAL;
>  
> -	req_int = (u64)(fi->interval.numerator * 10000) /
> +	req_int = (u64)fi->interval.numerator * 10000 /
>  		fi->interval.denominator;

This requires do_div(). I've applied the patch with this change:

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 88276dba828d..5bea31cd41aa 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1136,8 +1136,8 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
 	if (fi->interval.denominator == 0)
 		return -EINVAL;
 
-	req_int = (u64)fi->interval.numerator * 10000 /
-		fi->interval.denominator;
+	req_int = (u64)fi->interval.numerator * 10000;
+	do_div(req_int, fi->interval.denominator);
 
 	for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
 		const struct ov965x_interval *iv = &ov965x_intervals[i];

>  
>  	for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH 5/8] pci: cx88-input: fix potential integer overflow

[Gustavo A. R. Silva]

Cast ir->polling to ktime_t in order to avoid a potential integer
overflow. This variable is being used in a context that expects
an expression of type ktime_t (s64).

Addresses-Coverity-ID: 1392628 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1392630 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/pci/cx88/cx88-input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c
index 4e9953e..096b350 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -180,7 +180,7 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer *timer)
 	struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer);
 
 	cx88_ir_handle_key(ir);
-	missed = hrtimer_forward_now(&ir->timer, ir->polling * 1000000);
+	missed = hrtimer_forward_now(&ir->timer, (ktime_t)ir->polling * 1000000);
 	if (missed > 1)
 		ir_dprintk("Missed ticks %ld\n", missed - 1);
 
@@ -200,7 +200,7 @@ static int __cx88_ir_start(void *priv)
 	if (ir->polling) {
 		hrtimer_init(&ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 		ir->timer.function = cx88_ir_work;
-		hrtimer_start(&ir->timer, ir->polling * 1000000,
+		hrtimer_start(&ir->timer, (ktime_t)ir->polling * 1000000,
 			      HRTIMER_MODE_REL);
 	}
 	if (ir->sampling) {
-- 
2.7.4

[PATCH 6/8] rockchip/rga: fix potential integer overflow in rga_buf_map

[Gustavo A. R. Silva]

Cast p to dma_addr_t in order to avoid a potential integer overflow.
This variable is being used in a context that expects an expression
of type dma_addr_t (u64).

Addresses-Coverity-ID: 1458347 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/rockchip/rga/rga-buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
index 49cacc7..3dd29b2 100644
--- a/drivers/media/platform/rockchip/rga/rga-buf.c
+++ b/drivers/media/platform/rockchip/rga/rga-buf.c
@@ -140,7 +140,7 @@ void rga_buf_map(struct vb2_buffer *vb)
 		address = sg_phys(sgl);
 
 		for (p = 0; p < len; p++) {
-			dma_addr_t phys = address + (p << PAGE_SHIFT);
+			dma_addr_t phys = address + ((dma_addr_t)p << PAGE_SHIFT);
 
 			pages[mapped_size + p] = phys;
 		}
-- 
2.7.4

[PATCH 7/8] platform: sh_veu: fix potential integer overflow in sh_veu_colour_offset

[Gustavo A. R. Silva]

Cast left and top to dma_addr_t in order to avoid a potential integer
overflow. This variable is being used in a context that expects an
expression of type dma_addr_t (u64).

Addresses-Coverity-ID: 1056807 ("Unintentional integer overflow")
Addresses-Coverity-ID: 1056808 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/sh_veu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
index 976ea0b..e2795d0 100644
--- a/drivers/media/platform/sh_veu.c
+++ b/drivers/media/platform/sh_veu.c
@@ -520,8 +520,8 @@ static void sh_veu_colour_offset(struct sh_veu_dev *veu, struct sh_veu_vfmt *vfm
 	/* dst_left and dst_top validity will be verified in CROP / COMPOSE */
 	unsigned int left = vfmt->frame.left & ~0x03;
 	unsigned int top = vfmt->frame.top;
-	dma_addr_t offset = ((left * veu->vfmt_out.fmt->depth) >> 3) +
-		top * veu->vfmt_out.bytesperline;
+	dma_addr_t offset = (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3) +
+			    (dma_addr_t)top * veu->vfmt_out.bytesperline;
 	unsigned int y_line;
 
 	vfmt->offset_y = offset;
-- 
2.7.4

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

[Gustavo A. R. Silva]

Cast len to const u64 in order to avoid a potential integer
overflow. This variable is being used in a context that expects
an expression of type const u64.

Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/vivid/vivid-cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
index b55d278..30240ab 100644
--- a/drivers/media/platform/vivid/vivid-cec.c
+++ b/drivers/media/platform/vivid/vivid-cec.c
@@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts,
 	if (adap == NULL)
 		return;
 	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
-			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
+			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));
 	cec_queue_pin_cec_event(adap, false, ts);
 	ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
 	cec_queue_pin_cec_event(adap, true, ts);
-- 
2.7.4

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

[Hans Verkuil]

Hi Gustavo,

On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote:
> Cast len to const u64 in order to avoid a potential integer
> overflow. This variable is being used in a context that expects
> an expression of type const u64.
> 
> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/media/platform/vivid/vivid-cec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
> index b55d278..30240ab 100644
> --- a/drivers/media/platform/vivid/vivid-cec.c
> +++ b/drivers/media/platform/vivid/vivid-cec.c
> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts,
>  	if (adap == NULL)
>  		return;
>  	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
> -			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
> +			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));

This makes no sense. Certainly the const part is pointless. And given that
len is always <= 16 there definitely is no overflow.

I don't really want this cast in the code.

Sorry,

	Hans

>  	cec_queue_pin_cec_event(adap, false, ts);
>  	ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
>  	cec_queue_pin_cec_event(adap, true, ts);
> 

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

[Gustavo A. R. Silva]

Hi Hans,

Quoting Hans Verkuil <hverkuil@xs4all.nl>:

> Hi Gustavo,
>
> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote:
>> Cast len to const u64 in order to avoid a potential integer
>> overflow. This variable is being used in a context that expects
>> an expression of type const u64.
>>
>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/media/platform/vivid/vivid-cec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/vivid/vivid-cec.c  
>> b/drivers/media/platform/vivid/vivid-cec.c
>> index b55d278..30240ab 100644
>> --- a/drivers/media/platform/vivid/vivid-cec.c
>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct  
>> cec_adapter *adap, ktime_t ts,
>>  	if (adap == NULL)
>>  		return;
>>  	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>> -			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>> +			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>
> This makes no sense. Certainly the const part is pointless. And given that
> len is always <= 16 there definitely is no overflow.
>

Yeah, I understand your point and I know there is no chance of an  
overflow in this particular case.

> I don't really want this cast in the code.
>
> Sorry,
>

I'm working through all the Linux kernel Coverity reports, and I  
thought of sending a patch for this because IMHO it doesn't hurt to  
give the compiler complete information about the arithmetic in which  
an expression is intended to be evaluated.

I agree that the _const_ part is a bit odd. What do you think about  
the cast to u64 alone?

I appreciate your feedback.

Thanks
--
Gustavo

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

[Hans Verkuil]

On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote:
> Hi Hans,
> 
> Quoting Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> Hi Gustavo,
>>
>> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote:
>>> Cast len to const u64 in order to avoid a potential integer
>>> overflow. This variable is being used in a context that expects
>>> an expression of type const u64.
>>>
>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>> ---
>>>  drivers/media/platform/vivid/vivid-cec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c  
>>> b/drivers/media/platform/vivid/vivid-cec.c
>>> index b55d278..30240ab 100644
>>> --- a/drivers/media/platform/vivid/vivid-cec.c
>>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct  
>>> cec_adapter *adap, ktime_t ts,
>>>  	if (adap == NULL)
>>>  		return;
>>>  	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>>> -			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>> +			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>
>> This makes no sense. Certainly the const part is pointless. And given that
>> len is always <= 16 there definitely is no overflow.
>>
> 
> Yeah, I understand your point and I know there is no chance of an  
> overflow in this particular case.
> 
>> I don't really want this cast in the code.
>>
>> Sorry,
>>
> 
> I'm working through all the Linux kernel Coverity reports, and I  
> thought of sending a patch for this because IMHO it doesn't hurt to  
> give the compiler complete information about the arithmetic in which  
> an expression is intended to be evaluated.
> 
> I agree that the _const_ part is a bit odd. What do you think about  
> the cast to u64 alone?

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.

It definitely needs a comment that this fixes a bogus Coverity report.

Regards,

	Hans

> 
> I appreciate your feedback.
> 
> Thanks
> --
> Gustavo
> 
> 
> 
> 
> 

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@xs4all.nl>:

> On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote:
>> Hi Hans,
>>
>> Quoting Hans Verkuil <hverkuil@xs4all.nl>:
>>
>>> Hi Gustavo,
>>>
>>> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote:
>>>> Cast len to const u64 in order to avoid a potential integer
>>>> overflow. This variable is being used in a context that expects
>>>> an expression of type const u64.
>>>>
>>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>>  drivers/media/platform/vivid/vivid-cec.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c
>>>> b/drivers/media/platform/vivid/vivid-cec.c
>>>> index b55d278..30240ab 100644
>>>> --- a/drivers/media/platform/vivid/vivid-cec.c
>>>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>>>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct
>>>> cec_adapter *adap, ktime_t ts,
>>>>  	if (adap == NULL)
>>>>  		return;
>>>>  	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>>>> -			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>>> +			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>>
>>> This makes no sense. Certainly the const part is pointless. And given that
>>> len is always <= 16 there definitely is no overflow.
>>>
>>
>> Yeah, I understand your point and I know there is no chance of an
>> overflow in this particular case.
>>
>>> I don't really want this cast in the code.
>>>
>>> Sorry,
>>>
>>
>> I'm working through all the Linux kernel Coverity reports, and I
>> thought of sending a patch for this because IMHO it doesn't hurt to
>> give the compiler complete information about the arithmetic in which
>> an expression is intended to be evaluated.
>>
>> I agree that the _const_ part is a bit odd. What do you think about
>> the cast to u64 alone?
>
> 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

and avoid the cast in the middle.

What do you think?

> 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")

Certainly, I've run across multiple false positives as in this case,  
but I have also fixed many actual bugs thanks to the Coverity reports.  
So I think in general it is valuable to take a look into these  
reports, either if they spot actual bugs or promote code correctness.

Thanks
--
Gustavo

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

[Hans Verkuil]

On 01/30/18 11:55, Gustavo A. R. Silva wrote:
> 
> Quoting Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote:
>>> Hi Hans,
>>>
>>> Quoting Hans Verkuil <hverkuil@xs4all.nl>:
>>>
>>>> Hi Gustavo,
>>>>
>>>> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote:
>>>>> Cast len to const u64 in order to avoid a potential integer
>>>>> overflow. This variable is being used in a context that expects
>>>>> an expression of type const u64.
>>>>>
>>>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow")
>>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>>> ---
>>>>>  drivers/media/platform/vivid/vivid-cec.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c
>>>>> b/drivers/media/platform/vivid/vivid-cec.c
>>>>> index b55d278..30240ab 100644
>>>>> --- a/drivers/media/platform/vivid/vivid-cec.c
>>>>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>>>>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct
>>>>> cec_adapter *adap, ktime_t ts,
>>>>>  	if (adap == NULL)
>>>>>  		return;
>>>>>  	ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>>>>> -			       len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>>>> +			       (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>>>
>>>> This makes no sense. Certainly the const part is pointless. And given that
>>>> len is always <= 16 there definitely is no overflow.
>>>>
>>>
>>> Yeah, I understand your point and I know there is no chance of an
>>> overflow in this particular case.
>>>
>>>> I don't really want this cast in the code.
>>>>
>>>> Sorry,
>>>>
>>>
>>> I'm working through all the Linux kernel Coverity reports, and I
>>> thought of sending a patch for this because IMHO it doesn't hurt to
>>> give the compiler complete information about the arithmetic in which
>>> an expression is intended to be evaluated.
>>>
>>> I agree that the _const_ part is a bit odd. What do you think about
>>> the cast to u64 alone?
>>
>> 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 * ...

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

> 
>> 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 :-)

> 
> Certainly, I've run across multiple false positives as in this case,  
> but I have also fixed many actual bugs thanks to the Coverity reports.  
> So I think in general it is valuable to take a look into these  
> reports, either if they spot actual bugs or promote code correctness.

Regards,

	Hans

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

Thanks
--
Gustavo

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

[Hans Verkuil]

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

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@xs4all.nl>:

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

OK. That's fine.

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

OK. I got it.

Thanks for all the feedback, Hans.
--
Gustavo