Re: [PATCH] gpiolib: cdev: zero padding during conversion to gpioline_info_changed

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Jun 18, 2021 at 03:45:11PM -0700, Gabriel Knezek wrote:

Probably should've been [PATCH v2], despite the subject rename.

And CC the maintainers (LinusW and Bart here) and past reviewers (Andy
and me).

> From: Gabriel Knezek <gabeknez@xxxxxxxxxxxxx>
> 

A second From: header?  With a different address?
Perhaps you could pick one?
Neither git nor checkpatch.pl seem to mind, but it is odd.

> When userspace requests a GPIO v1 line info changed event, the kernel
> populates and returns the gpioline_info_changed structure. That structure
> contains 5 words of padding at the end of the structure that are not
> initialized before being returned to usermode:
> 

Avoid code snippets in checkin comments - try to stick to plain English.
So replace "the kernel" with "lineinfo_watch_read()" and drop the
snippets.

And "usermode" -> "userspace".

> struct gpioline_info_changed {
>                 struct gpioline_info info;
>                 __u64 timestamp;
>                 __u32 event_type;
>                 __u32 padding[5]; /* for future use */
> };
> 
> Which is used here in the lineinfo_watch_read routine:
> } else {
>                 struct gpioline_info_changed event_v1;
>                 gpio_v2_line_info_changed_to_v1(&event, &event_v1);
>                 if (copy_to_user(buf + bytes_read, &event_v1,
>                                                 event_size))
>                                 return -EFAULT;
> 
> Fix this by zeroing the structure in gpio_v2_line_info_change_to_v1
> before populating its contents.
> 

Make that "gpio_v2_line_info_change_to_v1()" as you are referring to a
function.

And maybe "Fix this by zeroing" to just "Zero"?

> Signed-off-by: Gabriel Knezek <gabeknez@xxxxxxxxxxxxx>

You should retain the Fixes tag from v1 - it is important to identify
where this patch will need to be backported to.
And include at least the first twelve characters of the SHA-1 [1].

> ---
>  drivers/gpio/gpiolib-cdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index ee5903aac497..af68532835fe 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1865,6 +1865,7 @@ static void gpio_v2_line_info_changed_to_v1(
>  		struct gpio_v2_line_info_changed *lic_v2,
>  		struct gpioline_info_changed *lic_v1)
>  {
> +	memset(lic_v1, 0, sizeof(*lic_v1));
>  	gpio_v2_line_info_to_v1(&lic_v2->info, &lic_v1->info);
>  	lic_v1->timestamp = lic_v2->timestamp_ns;
>  	lic_v1->event_type = lic_v2->event_type;
> -- 
> 2.25.1
> 

Still good with this bit ;)

Cheers,
Kent.
[1] https://www.kernel.org/doc/html/v5.12/process/submitting-patches.html