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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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