[PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[Yizhuo]

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 <yzhai003@ucr.edu>
---
 drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index ad5b25b89699..66a34fb9e6b2 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -790,7 +790,7 @@ static int ctrl_cx2341x_set(struct pvr2_ctrl *cptr,int m,int v)
 
 static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
 {
-	struct v4l2_queryctrl qctrl;
+	struct v4l2_queryctrl qctrl = {};
 	struct pvr2_ctl_info *info;
 	qctrl.id = cptr->info->v4l_id;
 	cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
-- 
2.17.1

Re: [PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[isely]

Acked-by: Mike Isely <isely@pobox.com>

On Mon, 26 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 <yzhai003@ucr.edu>
> ---
>  drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> index ad5b25b89699..66a34fb9e6b2 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> @@ -790,7 +790,7 @@ static int ctrl_cx2341x_set(struct pvr2_ctrl *cptr,int m,int v)
>  
>  static unsigned int ctrl_cx2341x_getv4lflags(struct pvr2_ctrl *cptr)
>  {
> -	struct v4l2_queryctrl qctrl;
> +	struct v4l2_queryctrl qctrl = {};
>  	struct pvr2_ctl_info *info;
>  	qctrl.id = cptr->info->v4l_id;
>  	cx2341x_ctrl_query(&cptr->hdw->enc_ctl_state,&qctrl);
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[Yizhuo Zhai]

Thanks for the correction, let me send a new patch then.

On Mon, Aug 26, 2019 at 5:09 AM Hans Verkuil <hverkuil@xs4all.nl> 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 <yzhai003@ucr.edu>
> > ---
> >  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

Re: [PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[Hans Verkuil]

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 <yzhai003@ucr.edu>
> ---
>  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
> 

Re: [PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[isely]

Acked-by: Mike Isely <isely@pobox.com>

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 <yzhai003@ucr.edu>
> ---
>  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

Re: [PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

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 <lkp@intel.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51782 bytes --]

[PATCH] [media] pvrusb2: qctrl.flag will be uninitlaized if cx2341x_ctrl_query() returns error code

[Yizhuo]

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 <yzhai003@ucr.edu>
---
 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