Re: [PATCH] [media] tvp686x: Don't go past array

[Hans Verkuil <hverkuil@xs4all.nl>]

On 04/25/2016 02:40 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 25 Apr 2016 13:36:57 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Since my patch exchanges the sparse warning with a smatch warning, it's
>> OK to take this one, with a few corrections:
>>
>> Please update the subject line (it says tvp686x instead of tw686x).
> 
> Gah...
> 
>>
>> On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
>>> Depending on the compiler version, currently it produces the
>>> following warnings:
>>> 	tw686x-video.c: In function 'tw686x_video_init':
>>> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
>>>
>>> This is actually bogus with the current code, as it currently
>>> hardcodes the framerate to 30 frames/sec, however a potential
>>> use after the array size could happen when the driver adds support
>>> for setting the framerate. So, fix it.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>> ---
>>>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
>>> index 118e9fac9f28..1ff59084ce08 100644
>>> --- a/drivers/media/pci/tw686x/tw686x-video.c
>>> +++ b/drivers/media/pci/tw686x/tw686x-video.c
>>> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>>>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>>>  	};
>>>  
>>> -	unsigned int i =
>>> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
>>> +	unsigned int i;
>>> +
>>> +	if (std & V4L2_STD_625_50) {  
>>
>> Please test against 525_60 since that is the recommended test.
> 
> Both ways should work, but I'm OK with such change.
> 
>>
>>> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))  
>>
>> Please don't use 'unlikely'. It's pointless for code that is rarely used.
> 
> OK.
> 
>>
>> Actually, the code is wrong: i is uninitialized here.
>>
>> It should be fps >= ARRAY_SIZE(std_625_50).
>>
>> In fact, I'd write it like this:
>>
>> 		i = std_625_50[(fps >= ARRAY_SIZE(std_625_50) ? 24 : fps];
> 
> I really don't like the above, as it has an unexplained magic
> number on it. Also, "24" is wrong there.
> 
> So, I would go to the following enclosed patch.

Looks good to me. Acked below. Amazing how many bugs one can make in one
simple patch...

> 
> Ezequiel,
> 
> Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> As the device seems to support setting the fps, you should be adding
> support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> 
> On both ioctls, the driver should return the actual framerate used.
> So, you'll need to add a code that would convert from the 15 possible
> framerate converter register settings to v4l2_fract.
> 
>>
>>> +			i = 14;		/* 25 fps */
>>> +		else
>>> +			i = std_625_50[fps];
>>> +	} else {
>>> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
>>> +			i = 0;		/* 30 fps */
>>> +		else
>>> +			i = std_525_60[fps];
>>> +	}
>>>  
>>>  	return map[i];
>>>  }
>>>   
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks,
> Mauro
> 
> -
> 
> [media] tw686x: Don't go past array
> 
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..9468fda69f3d 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,17 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_525_60) {
> +		if (fps > ARRAY_SIZE(std_525_60))
> +			fps = 30;
> +		i = std_525_60[fps];
> +	} else {
> +		if (fps > ARRAY_SIZE(std_625_50))
> +			fps = 25;
> +		i = std_625_50[fps];
> +	}
>  
>  	return map[i];
>  }
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>