[PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

[PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

[Dan Carpenter]

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>

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);
 }
 

[PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

[Dan Carpenter]

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>

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);
 }
 

Re: [PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

[Hans Verkuil]

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);
>  }
>  
> 

Re: [PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()

[Hans Verkuil]

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);
>  }
>  
> 

[PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that temp_index can be negative.  I've made
cgf->num_outputs unsigned to fix this issue.

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

diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 79a566d7defd..180a05e91497 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
 	struct encoder_config_info *ext_encoders;
 	/* amplifier information goes here */
 	struct amp_config_info *amp;
-	int num_outputs;
+	unsigned int num_outputs;
 	/* Order is venc outputs followed by LCD and then external encoders */
 	struct vpbe_output *outputs;
 };

[PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that temp_index can be negative.  I've made
cgf->num_outputs unsigned to fix this issue.

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

diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 79a566d7defd..180a05e91497 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
 	struct encoder_config_info *ext_encoders;
 	/* amplifier information goes here */
 	struct amp_config_info *amp;
-	int num_outputs;
+	unsigned int num_outputs;
 	/* Order is venc outputs followed by LCD and then external encoders */
 	struct vpbe_output *outputs;
 };

Re: [PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Hans Verkuil]

On 25/05/18 15:12, Dan Carpenter wrote:
> In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
> the problem is that temp_index can be negative.  I've made
> cgf->num_outputs unsigned to fix this issue.

Shouldn't temp_index also be made unsigned? It certainly would make a lot of
sense to do that.

Regards,

	Hans

> 
> Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: fix it a different way
> 
> diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
> index 79a566d7defd..180a05e91497 100644
> --- a/include/media/davinci/vpbe.h
> +++ b/include/media/davinci/vpbe.h
> @@ -92,7 +92,7 @@ struct vpbe_config {
>  	struct encoder_config_info *ext_encoders;
>  	/* amplifier information goes here */
>  	struct amp_config_info *amp;
> -	int num_outputs;
> +	unsigned int num_outputs;
>  	/* Order is venc outputs followed by LCD and then external encoders */
>  	struct vpbe_output *outputs;
>  };
> 

Re: [PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Hans Verkuil]

On 25/05/18 15:12, Dan Carpenter wrote:
> In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
> the problem is that temp_index can be negative.  I've made
> cgf->num_outputs unsigned to fix this issue.

Shouldn't temp_index also be made unsigned? It certainly would make a lot of
sense to do that.

Regards,

	Hans

> 
> Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: fix it a different way
> 
> diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
> index 79a566d7defd..180a05e91497 100644
> --- a/include/media/davinci/vpbe.h
> +++ b/include/media/davinci/vpbe.h
> @@ -92,7 +92,7 @@ struct vpbe_config {
>  	struct encoder_config_info *ext_encoders;
>  	/* amplifier information goes here */
>  	struct amp_config_info *amp;
> -	int num_outputs;
> +	unsigned int num_outputs;
>  	/* Order is venc outputs followed by LCD and then external encoders */
>  	struct vpbe_output *outputs;
>  };
> 

Re: [PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

On Fri, May 25, 2018 at 03:16:21PM +0200, Hans Verkuil wrote:
> On 25/05/18 15:12, Dan Carpenter wrote:
> > In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
> > the problem is that temp_index can be negative.  I've made
> > cgf->num_outputs unsigned to fix this issue.
> 
> Shouldn't temp_index also be made unsigned? It certainly would make a lot of
> sense to do that.

Yeah, sure.  It doesn't make any difference in terms of runtime but it's
probably cleaner.

regards,
dan carpenter

Re: [PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

On Fri, May 25, 2018 at 03:16:21PM +0200, Hans Verkuil wrote:
> On 25/05/18 15:12, Dan Carpenter wrote:
> > In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
> > the problem is that temp_index can be negative.  I've made
> > cgf->num_outputs unsigned to fix this issue.
> 
> Shouldn't temp_index also be made unsigned? It certainly would make a lot of
> sense to do that.

Yeah, sure.  It doesn't make any difference in terms of runtime but it's
probably cleaner.

regards,
dan carpenter

Re: [PATCH v3] media: davinci vpbe: array underflow in vpbe_enum_outputs()

[Lad, Prabhakar]

On Mon, May 28, 2018 at 7:26 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
> the problem is that temp_index can be negative.  I've changed the types
> to unsigned to fix this issue.
>
> Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: fix it a different way
> v3: change everything to unsigned because that's the right thing to do
>     and looks nicer.
>

Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Regards,
--Prabhakar Lad

> diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
> index 79a566d7defd..180a05e91497 100644
> --- a/include/media/davinci/vpbe.h
> +++ b/include/media/davinci/vpbe.h
> @@ -92,7 +92,7 @@ struct vpbe_config {
>         struct encoder_config_info *ext_encoders;
>         /* amplifier information goes here */
>         struct amp_config_info *amp;
> -       int num_outputs;
> +       unsigned int num_outputs;
>         /* Order is venc outputs followed by LCD and then external encoders */
>         struct vpbe_output *outputs;
>  };
> diff --git a/drivers/media/platform/davinci/vpbe.c b/drivers/media/platform/davinci/vpbe.c
> index 18c035ef84cf..c6fee53bff4d 100644
> --- a/drivers/media/platform/davinci/vpbe.c
> +++ b/drivers/media/platform/davinci/vpbe.c
> @@ -126,7 +126,7 @@ static int vpbe_enum_outputs(struct vpbe_device *vpbe_dev,
>                              struct v4l2_output *output)
>  {
>         struct vpbe_config *cfg = vpbe_dev->cfg;
> -       int temp_index = output->index;
> +       unsigned int temp_index = output->index;
>
>         if (temp_index >= cfg->num_outputs)
>                 return -EINVAL;

[PATCH v3 resend] media: davinci/vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that "temp_index" can be negative.  This patch changes
the types to unsigned to address this array underflow bug.

Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
---
I sent this patch last May but somehow the spam filters on the lists
must have eaten it.  I didn't get a copy from the kernel-janitors list.
The only trace I have of my original patch is that the maintainer Acked
it.  Resending.

v2: In the first version, I clamped output->index to 0-INT_MAX for every
    driver.  In v2, I only changed the vpbe.h driver header file.
v3: In v3 I changed the header and the .c file (All three versions of
    patch "worked", they just had philosophical and style issues).

 drivers/media/platform/davinci/vpbe.c | 2 +-
 include/media/davinci/vpbe.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpbe.c b/drivers/media/platform/davinci/vpbe.c
index 8339163a5231..4e24f5d781f4 100644
--- a/drivers/media/platform/davinci/vpbe.c
+++ b/drivers/media/platform/davinci/vpbe.c
@@ -104,7 +104,7 @@ static int vpbe_enum_outputs(struct vpbe_device *vpbe_dev,
 			     struct v4l2_output *output)
 {
 	struct vpbe_config *cfg = vpbe_dev->cfg;
-	int temp_index = output->index;
+	unsigned int temp_index = output->index;
 
 	if (temp_index >= cfg->num_outputs)
 		return -EINVAL;
diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 5c31a7682492..f76d2f25a824 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
 	struct encoder_config_info *ext_encoders;
 	/* amplifier information goes here */
 	struct amp_config_info *amp;
-	int num_outputs;
+	unsigned int num_outputs;
 	/* Order is venc outputs followed by LCD and then external encoders */
 	struct vpbe_output *outputs;
 };
-- 
2.18.0

[PATCH v3 resend] media: davinci/vpbe: array underflow in vpbe_enum_outputs()

[Dan Carpenter]

In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that "temp_index" can be negative.  This patch changes
the types to unsigned to address this array underflow bug.

Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
---
I sent this patch last May but somehow the spam filters on the lists
must have eaten it.  I didn't get a copy from the kernel-janitors list.
The only trace I have of my original patch is that the maintainer Acked
it.  Resending.

v2: In the first version, I clamped output->index to 0-INT_MAX for every
    driver.  In v2, I only changed the vpbe.h driver header file.
v3: In v3 I changed the header and the .c file (All three versions of
    patch "worked", they just had philosophical and style issues).

 drivers/media/platform/davinci/vpbe.c | 2 +-
 include/media/davinci/vpbe.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpbe.c b/drivers/media/platform/davinci/vpbe.c
index 8339163a5231..4e24f5d781f4 100644
--- a/drivers/media/platform/davinci/vpbe.c
+++ b/drivers/media/platform/davinci/vpbe.c
@@ -104,7 +104,7 @@ static int vpbe_enum_outputs(struct vpbe_device *vpbe_dev,
 			     struct v4l2_output *output)
 {
 	struct vpbe_config *cfg = vpbe_dev->cfg;
-	int temp_index = output->index;
+	unsigned int temp_index = output->index;
 
 	if (temp_index >= cfg->num_outputs)
 		return -EINVAL;
diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 5c31a7682492..f76d2f25a824 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
 	struct encoder_config_info *ext_encoders;
 	/* amplifier information goes here */
 	struct amp_config_info *amp;
-	int num_outputs;
+	unsigned int num_outputs;
 	/* Order is venc outputs followed by LCD and then external encoders */
 	struct vpbe_output *outputs;
 };
-- 
2.18.0