Re: [PATCH 1/6] media: v4l2-dev: Add range check for vdev->minor

[Hans Verkuil]

On 29/04/2026 09:43, Sakari Ailus wrote:
> Hi Ricardo,
> 
> On Tue, Apr 28, 2026 at 12:41:07PM +0000, Ricardo Ribalda wrote:
>> If the fixed minor ranges are not properly set we could end up in a
>> situation where the calculated minor is invalid. Add a check for this in
>> the code.
>>
>> This check also fixes the following smatch warning:
>>
>> drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
>> drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
>> drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
>>
>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 6ce623a1245a..a731ffdb91ee 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -1032,6 +1032,12 @@ int __video_register_device(struct video_device *vdev,
>>  	vdev->minor = i + minor_offset;
>>  	vdev->num = nr;
>>  
>> +	if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> 
> Could this be combined with the should-not-happen case below? The error
> handling is the same (releasing the mutex) and the error code could be as
> well. I think the message can be just as well removed as we have a
> WARN_ON() here anyway.
> 
> I wonder what Hans thinks.

I actually prefer to keep it separate. If you combine it, then it is hard
to see which of the two possibilities is actually wrong (out of range or
minor in use). And this function sits at the core of V4L2, so it's OK to
be a bit more verbose.

I do agree that a pr_err after a WARN_ON is not needed.

Regards,

	Hans

> 
>> +		mutex_unlock(&videodev_lock);
>> +		pr_err("invalid minor. Check ranges.\n");
>> +		return -EINVAL;
>> +	}
>> +
>>  	/* Should not happen since we thought this minor was free */
>>  	if (WARN_ON(video_devices[vdev->minor])) {
>>  		mutex_unlock(&videodev_lock);
>>
>