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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Wed, Apr 29, 2026 at 09:53:41AM +0200, Hans Verkuil wrote:
> 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@chromium.org>
> >> ---
> >>  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

Not if they're on different lines as you get the line number with
WARN_ON(). Keeping such code small has benefits.

That being said, I certainly have no strong opinion about this.

> be a bit more verbose.
> 
> I do agree that a pr_err after a WARN_ON is not needed.

-- 
Regards,

Sakari Ailus