diff mbox

media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

Message ID 20180517090550.GB4250@mwanda (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Dan Carpenter May 17, 2018, 9:05 a.m. UTC 
  My Smatch allmodconfig build only detects one function implementing
vpbe_device_ops->enum_outputs and that's vpbe_enum_outputs().  The
problem really happens in that function when we do:

	int temp_index = output->index;

	if (temp_index >= cfg->num_outputs)
		return -EINVAL;

Unfortunately, both temp_index and cfg->num_outputs are type int so we
have a potential read before the start of the array if "temp_index" is
negative.

I could have fixed the bug in that function but it's more secure and
future proof to block that bug earlier in a central place.  There is no
one who need p->index to be more than INT_MAX.

Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Hans Verkuil May 25, 2018, 9:06 a.m. UTC | #1 
On 17/05/18 11:05, Dan Carpenter wrote:
> My Smatch allmodconfig build only detects one function implementing
> vpbe_device_ops->enum_outputs and that's vpbe_enum_outputs().  The
> problem really happens in that function when we do:
> 
> 	int temp_index = output->index;
> 
> 	if (temp_index >= cfg->num_outputs)
> 		return -EINVAL;
> 
> Unfortunately, both temp_index and cfg->num_outputs are type int so we
> have a potential read before the start of the array if "temp_index" is
> negative.

Why not fix it in this driver? Make num_outputs unsigned, as it should be.

I really don't like having a random index check in the core. If we ever
want to do such things in the core, then it needs to be implemented
consistently for all ioctls that do something similar.

Regards,

	Hans

> 
> I could have fixed the bug in that function but it's more secure and
> future proof to block that bug earlier in a central place.  There is no
> one who need p->index to be more than INT_MAX.
> 
> Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a40dbec271f1..115757ab8bc0 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1099,6 +1099,9 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>  		p->capabilities |= V4L2_OUT_CAP_STD;
>  
> +	if (p->index > INT_MAX)
> +		return -EINVAL;
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
>
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a40dbec271f1..115757ab8bc0 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1099,6 +1099,9 @@  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
 	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
 		p->capabilities |= V4L2_OUT_CAP_STD;
 
+	if (p->index > INT_MAX)
+		return -EINVAL;
+
 	return ops->vidioc_enum_output(file, fh, p);
 }