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

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

[Gabriel Knezek]

From: Gabriel Knezek <gabeknez@microsoft.com>

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:

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.

Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com>
---
 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

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

[Kent Gibson]

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@microsoft.com>
> 

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@microsoft.com>

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

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

[Gabriel Knezek]

On Sat, Jun 19, 2021 at 12:11:44PM +0800, Kent Gibson wrote:
> 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).
> 

Sorry about that. When sending the revised patch, should I change it to v2?
Or perhaps v3?

> > From: Gabriel Knezek <gabeknez@microsoft.com>
> > 
> 
> 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.
> 

Apologies for that as well. git send-email doesn't play well with Exchange, so
after talking to coworkers it turns out we have a separate email server for
sending patches which I wasn't aware of before I sent the first one. :-/ Sorry
for the confusion; I'll stick to the @linux address.

> 
> > Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com>
> 
> 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].
> 

Oh shoot. Sorry about that as well. I need to make a checklist.
Thanks for the link; I did read it before submitting the second patch,
but I clearly missed that part.

> 
> Still good with this bit ;)
> 
> Cheers,
> Kent.

Thanks for the kind help. I promise I'll update our wiki with these
learnings so hopefully dealing with the next person from our team submitting
a patch won't be so painful.

-Gabe

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

[Andy Shevchenko]

On Sat, Jun 19, 2021 at 8:49 PM Gabriel Knezek
<gabeknez@linux.microsoft.com> wrote:
> On Sat, Jun 19, 2021 at 12:11:44PM +0800, Kent Gibson wrote:
> > 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).
> Sorry about that. When sending the revised patch, should I change it to v2?
> Or perhaps v3?

Yes, v3 with a changelog for each previous version.

...

> > > From: Gabriel Knezek <gabeknez@microsoft.com>
> > >
> >
> > 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.
>
> Apologies for that as well. git send-email doesn't play well with Exchange, so
> after talking to coworkers it turns out we have a separate email server for
> sending patches which I wasn't aware of before I sent the first one. :-/ Sorry
> for the confusion; I'll stick to the @linux address.

Don't forget to configure your Git accordingly (globally or per this project).
Note, you always can test this by doing

  git send-email --suppress-cc=all --to "Your Second <email@address>"

or so.

> > > Signed-off-by: Gabriel Knezek <gabeknez@microsoft.com>
> >
> > 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].
>
> Oh shoot. Sorry about that as well. I need to make a checklist.

The checklist is already made for you:
https://www.kernel.org/doc/html/latest/process/submit-checklist.html

> Thanks for the link; I did read it before submitting the second patch,
> but I clearly missed that part.
>
> > Still good with this bit ;)

> Thanks for the kind help. I promise I'll update our wiki with these
> learnings so hopefully dealing with the next person from our team submitting
> a patch won't be so painful.


-- 
With Best Regards,
Andy Shevchenko