[PATCH] Staging: media: atomisp: Remove duplicated argument to ||

[PATCH] Staging: media: atomisp: Remove duplicated argument to ||

[Georgiana Chelu]

get_user() copies a variable from user space to kernel space
put_user() copies a variable from kenel space to user space.

Remove duplicated argument to || because consecutive calls
of these functions with the same arguments will have the same
result.

Issue found by coccinelle script.

Signed-off-by: Georgiana Chelu <georgiana.chelu93@gmail.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
index 0592ac1..a27ecbe 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
@@ -432,8 +432,6 @@ static int get_atomisp_overlay32(struct atomisp_overlay *kp,
 				&up->blend_overlay_perc_u) ||
 		get_user(kp->blend_overlay_perc_v,
 				&up->blend_overlay_perc_v) ||
-		get_user(kp->blend_overlay_perc_u,
-				&up->blend_overlay_perc_u) ||
 		get_user(kp->overlay_start_x, &up->overlay_start_y))
 			return -EFAULT;
 
@@ -460,8 +458,6 @@ static int put_atomisp_overlay32(struct atomisp_overlay *kp,
 				&up->blend_overlay_perc_u) ||
 		put_user(kp->blend_overlay_perc_v,
 				&up->blend_overlay_perc_v) ||
-		put_user(kp->blend_overlay_perc_u,
-				&up->blend_overlay_perc_u) ||
 		put_user(kp->overlay_start_x, &up->overlay_start_y))
 			return -EFAULT;
 
-- 
2.7.4

Re: [PATCH] Staging: media: atomisp: Remove duplicated argument to ||

[Greg Kroah-Hartman]

On Fri, Sep 22, 2017 at 04:20:55AM -0700, Georgiana Chelu wrote:
> get_user() copies a variable from user space to kernel space
> put_user() copies a variable from kenel space to user space.
> 
> Remove duplicated argument to || because consecutive calls
> of these functions with the same arguments will have the same
> result.
> 
> Issue found by coccinelle script.
> 
> Signed-off-by: Georgiana Chelu <georgiana.chelu93@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> index 0592ac1..a27ecbe 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> @@ -432,8 +432,6 @@ static int get_atomisp_overlay32(struct atomisp_overlay *kp,
>  				&up->blend_overlay_perc_u) ||
>  		get_user(kp->blend_overlay_perc_v,
>  				&up->blend_overlay_perc_v) ||
> -		get_user(kp->blend_overlay_perc_u,
> -				&up->blend_overlay_perc_u) ||
>  		get_user(kp->overlay_start_x, &up->overlay_start_y))
>  			return -EFAULT;
>  

I don't understand, you are changing the output of the code here, right?
Why is this patch ok?

confused,

greg k-h

Re: [PATCH] Staging: media: atomisp: Remove duplicated argument to ||

[Georgiana Chelu]

On 29 September 2017 at 16:36, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Sep 22, 2017 at 04:20:55AM -0700, Georgiana Chelu wrote:
>> get_user() copies a variable from user space to kernel space
>> put_user() copies a variable from kenel space to user space.
>>
>> Remove duplicated argument to || because consecutive calls
>> of these functions with the same arguments will have the same
>> result.
>>
>> Issue found by coccinelle script.
>>
>> Signed-off-by: Georgiana Chelu <georgiana.chelu93@gmail.com>
>> ---
>>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
>> index 0592ac1..a27ecbe 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
>> @@ -432,8 +432,6 @@ static int get_atomisp_overlay32(struct atomisp_overlay *kp,
>>                               &up->blend_overlay_perc_u) ||
>>               get_user(kp->blend_overlay_perc_v,
>>                               &up->blend_overlay_perc_v) ||
>> -             get_user(kp->blend_overlay_perc_u,
>> -                             &up->blend_overlay_perc_u) ||
>>               get_user(kp->overlay_start_x, &up->overlay_start_y))
>>                       return -EFAULT;
>>
>
> I don't understand, you are changing the output of the code here, right?
> Why is this patch ok?

The output of the code is the same. This patch just removes the duplicate.
I know, it's hard to see it because the patch does not show the whole
if statement. The get_user() function does not change the value
of the variable from userspace. So, I thought that consecutive calls
over the same variable will have the same result.

Here is the whole if statement. The get_user is called twice for
the "blend_overlay_perc_u" and "up->blend_overlay_perc_u" variables.

if (!access_ok(VERIFY_READ, up, sizeof(struct atomisp_overlay32)) ||
get_user(frame, &up->frame) ||
get_user(kp->bg_y, &up->bg_y) ||
get_user(kp->bg_u, &up->bg_u) ||
get_user(kp->bg_v, &up->bg_v) ||
get_user(kp->blend_input_perc_y, &up->blend_input_perc_y) ||
get_user(kp->blend_input_perc_u, &up->blend_input_perc_u) ||
get_user(kp->blend_input_perc_v, &up->blend_input_perc_v) ||
get_user(kp->blend_overlay_perc_y, &up->blend_overlay_perc_y) ||
get_user(kp->blend_overlay_perc_u, &up->blend_overlay_perc_u) ||
get_user(kp->blend_overlay_perc_v, &up->blend_overlay_perc_v) ||
get_user(kp->blend_overlay_perc_u, &up->blend_overlay_perc_u) ||
get_user(kp->overlay_start_x, &up->overlay_start_y)) {...}


The reason behind the second change in this patch is the same.
The put_user() function is called twice to write the same value
into the same variable from userspace.

Thank you,
Georgiana

>
> confused,
>
> greg k-h

Re: [PATCH] Staging: media: atomisp: Remove duplicated argument to ||

[Greg Kroah-Hartman]

On Sun, Oct 01, 2017 at 01:47:41PM +0300, Georgiana Chelu wrote:
> On 29 September 2017 at 16:36, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Sep 22, 2017 at 04:20:55AM -0700, Georgiana Chelu wrote:
> >> get_user() copies a variable from user space to kernel space
> >> put_user() copies a variable from kenel space to user space.
> >>
> >> Remove duplicated argument to || because consecutive calls
> >> of these functions with the same arguments will have the same
> >> result.
> >>
> >> Issue found by coccinelle script.
> >>
> >> Signed-off-by: Georgiana Chelu <georgiana.chelu93@gmail.com>
> >> ---
> >>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> >> index 0592ac1..a27ecbe 100644
> >> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> >> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
> >> @@ -432,8 +432,6 @@ static int get_atomisp_overlay32(struct atomisp_overlay *kp,
> >>                               &up->blend_overlay_perc_u) ||
> >>               get_user(kp->blend_overlay_perc_v,
> >>                               &up->blend_overlay_perc_v) ||
> >> -             get_user(kp->blend_overlay_perc_u,
> >> -                             &up->blend_overlay_perc_u) ||
> >>               get_user(kp->overlay_start_x, &up->overlay_start_y))
> >>                       return -EFAULT;
> >>
> >
> > I don't understand, you are changing the output of the code here, right?
> > Why is this patch ok?
> 
> The output of the code is the same. This patch just removes the duplicate.
> I know, it's hard to see it because the patch does not show the whole
> if statement. The get_user() function does not change the value
> of the variable from userspace. So, I thought that consecutive calls
> over the same variable will have the same result.
> 
> Here is the whole if statement. The get_user is called twice for
> the "blend_overlay_perc_u" and "up->blend_overlay_perc_u" variables.
> 
> if (!access_ok(VERIFY_READ, up, sizeof(struct atomisp_overlay32)) ||
> get_user(frame, &up->frame) ||
> get_user(kp->bg_y, &up->bg_y) ||
> get_user(kp->bg_u, &up->bg_u) ||
> get_user(kp->bg_v, &up->bg_v) ||
> get_user(kp->blend_input_perc_y, &up->blend_input_perc_y) ||
> get_user(kp->blend_input_perc_u, &up->blend_input_perc_u) ||
> get_user(kp->blend_input_perc_v, &up->blend_input_perc_v) ||
> get_user(kp->blend_overlay_perc_y, &up->blend_overlay_perc_y) ||
> get_user(kp->blend_overlay_perc_u, &up->blend_overlay_perc_u) ||
> get_user(kp->blend_overlay_perc_v, &up->blend_overlay_perc_v) ||
> get_user(kp->blend_overlay_perc_u, &up->blend_overlay_perc_u) ||
> get_user(kp->overlay_start_x, &up->overlay_start_y)) {...}
> 
> 
> The reason behind the second change in this patch is the same.
> The put_user() function is called twice to write the same value
> into the same variable from userspace.

Ah, ok, care to put this type of info in the changelog itself to make it
more obvious?

thanks,

greg k-h