media: v4l2-ioctl: prevent underflow in v4l_enumoutput()
Subsystems (3)
PATCH 1/1 media: v4l2-ioctl: prevent underflow in v4l_enumoutput()
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.
drivers/media/v4l2-core/v4l2-ioctl.c +3 -0
@@ -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); }
Quoted 4 lines · Dan Carpenter
My Smatch allmodconfig build only detects one function implementing
vpbe_device_ops->enum_outputsand that'svpbe_enum_outputs(). The problem really happens in that function when we do:
Quoted 8 lines
int temp_index = output->index; if (temp_index >= cfg->num_outputs) return -EINVAL;Unfortunately, both
temp_indexandcfg->num_outputsare 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
Quoted 23 lines
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->indexto be more thanINT_MAX.Fixes 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")Signed-off-by Dan Carpenter <[email protected]>drivers/media/v4l2-core/v4l2-ioctl.c +3
-0@@ -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 1/1 media: davinci vpbe: array underflow in vpbe_enum_outputs()
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.
---
v2: fix it a different way
include/media/davinci/vpbe.h +1 -1
@@ -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()
Quoted 3 lines · Dan Carpenter
In
vpbe_enum_outputs()we check if (temp_index>=cfg->num_outputs) but the problem is thattemp_indexcan be negative. I've madecgf->num_outputsunsigned 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
Quoted 20 lines
Fixes 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")Signed-off-by Dan Carpenter <[email protected]>---
v2: fix it a different wayinclude/media/davinci/vpbe.h +1
-1@@ -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()
Quoted 7 lines · Hans Verkuil
Quoted 3 lines · Dan Carpenter
In
vpbe_enum_outputs()we check if (temp_index>=cfg->num_outputs) but the problem is thattemp_indexcan be negative. I've madecgf->num_outputsunsigned to fix this issue.Shouldn't
temp_indexalso 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()
In reply to Dan CarpenterQuoted 11 lines · Dan Carpenter
In
vpbe_enum_outputs()we check if (temp_index>=cfg->num_outputs) but the problem is thattemp_indexcan 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 <[email protected]>---
v2: fix it a different way
v3: change everything to unsigned because that's the right thing to do
and looks nicer.
Regards,
--Prabhakar Lad
Quoted 26 lines
include/media/davinci/vpbe.h +1
-1@@ -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; };drivers/media/platform/davinci/vpbe.c +1
-1@@ -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 RESEND v3 1/1 media: davinci/vpbe: array underflow in vpbe_enum_outputs()
In reply to Dan CarpenterIn 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.
---
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(-)
drivers/media/platform/davinci/vpbe.c +1 -1
@@ -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 ;
include/media/davinci/vpbe.h +1 -1
@@ -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; };
Signature · 1 line
2.18.0