Inside function ctrl_cx2341x_getv4lflags(), qctrl.flag
will be uninitlaized if cx2341x_ctrl_query() returns -EINVAL.
However, it will be used in the later if statement, which is
potentially unsafe.
Signed-off-by: Yizhuo <[email protected]>
---
drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index ad5b25b89699..1fa05971316a 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -793,6 +793,7 @@ static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
struct v4l2_queryctrl qctrl;
struct pvr2_ctl_info *info;
qctrl.id = cptr->info->v4l_id;
+ memset(&qctr, 0, sizeof(qctrl))
cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
/* Strip out the const so we can adjust a function pointer. It's
OK to do this here because we know this is a dynamically created
--
2.17.1
Hi Yizhuo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[cannot apply to v5.3-rc5 next-20190822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yizhuo/pvrusb2-qctrl-flag-will-be-uninitlaized-if-cx2341x_ctrl_query-returns-error-code/20190823-075303
base: git://linuxtv.org/media_tree.git master
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/media/usb/pvrusb2/pvrusb2-hdw.c: In function 'ctrl_cx2341x_getv4lflags':
>> drivers/media/usb/pvrusb2/pvrusb2-hdw.c:790:10: error: 'qctr' undeclared (first use in this function); did you mean 'qctrl'?
memset(&qctr, 0, sizeof(qctrl))
^~~~
qctrl
drivers/media/usb/pvrusb2/pvrusb2-hdw.c:790:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/media/usb/pvrusb2/pvrusb2-hdw.c:791:2: error: expected ';' before 'cx2341x_ctrl_query'
cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
^~~~~~~~~~~~~~~~~~
vim +790 drivers/media/usb/pvrusb2/pvrusb2-hdw.c
784
785 static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
786 {
787 struct v4l2_queryctrl qctrl;
788 struct pvr2_ctl_info *info;
789 qctrl.id = cptr->info->v4l_id;
> 790 memset(&qctr, 0, sizeof(qctrl))
> 791 cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
792 /* Strip out the const so we can adjust a function pointer. It's
793 OK to do this here because we know this is a dynamically created
794 control, so the underlying storage for the info pointer is (a)
795 private to us, and (b) not in read-only storage. Either we do
796 this or we significantly complicate the underlying control
797 implementation. */
798 info = (struct pvr2_ctl_info *)(cptr->info);
799 if (qctrl.flags & V4L2_CTRL_FLAG_READ_ONLY) {
800 if (info->set_value) {
801 info->set_value = NULL;
802 }
803 } else {
804 if (!(info->set_value)) {
805 info->set_value = ctrl_cx2341x_set;
806 }
807 }
808 return qctrl.flags;
809 }
810
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Acked-by: Mike Isely <[email protected]>
On Wed, 21 Aug 2019, Yizhuo wrote:
> Inside function ctrl_cx2341x_getv4lflags(), qctrl.flag
> will be uninitlaized if cx2341x_ctrl_query() returns -EINVAL.
> However, it will be used in the later if statement, which is
> potentially unsafe.
>
> Signed-off-by: Yizhuo <[email protected]>
> ---
> drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> index ad5b25b89699..1fa05971316a 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> @@ -793,6 +793,7 @@ static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
> struct v4l2_queryctrl qctrl;
> struct pvr2_ctl_info *info;
> qctrl.id = cptr->info->v4l_id;
> + memset(&qctr, 0, sizeof(qctrl))
> cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
> /* Strip out the const so we can adjust a function pointer. It's
> OK to do this here because we know this is a dynamically created
>
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
On 8/21/19 11:09 PM, Yizhuo wrote:
> Inside function ctrl_cx2341x_getv4lflags(), qctrl.flag
> will be uninitlaized if cx2341x_ctrl_query() returns -EINVAL.
> However, it will be used in the later if statement, which is
> potentially unsafe.
>
> Signed-off-by: Yizhuo <[email protected]>
> ---
> drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> index ad5b25b89699..1fa05971316a 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> @@ -793,6 +793,7 @@ static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
> struct v4l2_queryctrl qctrl;
> struct pvr2_ctl_info *info;
> qctrl.id = cptr->info->v4l_id;
> + memset(&qctr, 0, sizeof(qctrl))
Please compile test your patches! This doesn't compile due to a typo
(qctr -> qctrl).
Also, this would overwrite qctrl.id with 0, not what you want.
Instead, just do:
struct v4l2_queryctrl qctrl = {};
to initialize the struct with all 0.
Regards,
Hans
> cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
> /* Strip out the const so we can adjust a function pointer. It's
> OK to do this here because we know this is a dynamically created
>
Thanks for the correction, let me send a new patch then.
On Mon, Aug 26, 2019 at 5:09 AM Hans Verkuil <[email protected]> wrote:
>
> On 8/21/19 11:09 PM, Yizhuo wrote:
> > Inside function ctrl_cx2341x_getv4lflags(), qctrl.flag
> > will be uninitlaized if cx2341x_ctrl_query() returns -EINVAL.
> > However, it will be used in the later if statement, which is
> > potentially unsafe.
> >
> > Signed-off-by: Yizhuo <[email protected]>
> > ---
> > drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > index ad5b25b89699..1fa05971316a 100644
> > --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > @@ -793,6 +793,7 @@ static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
> > struct v4l2_queryctrl qctrl;
> > struct pvr2_ctl_info *info;
> > qctrl.id = cptr->info->v4l_id;
> > + memset(&qctr, 0, sizeof(qctrl))
>
> Please compile test your patches! This doesn't compile due to a typo
> (qctr -> qctrl).
>
> Also, this would overwrite qctrl.id with 0, not what you want.
>
> Instead, just do:
>
> struct v4l2_queryctrl qctrl = {};
>
> to initialize the struct with all 0.
>
> Regards,
>
> Hans
>
> > cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
> > /* Strip out the const so we can adjust a function pointer. It's
> > OK to do this here because we know this is a dynamically created
> >
>
--
Kind Regards,
Yizhuo Zhai
Computer Science, Graduate Student
University of California, Riverside