[PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko]

The introduced line even handling ABI in the commit

  61f922db7221 ("gpio: userspace ABI for reading GPIO line events")

missed the fact that 64-bit kernel may serve for 32-bit applications.
In such case the very first check in the lineevent_read() will fail
due to alignment differences.

To workaround this introduce lineeven_to_user() helper which returns actual
size of the structure and copies its content to user if asked.

Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: moved to just calculate size (Arnd, Kent), added good comment (Arnd)
 drivers/gpio/gpiolib-cdev.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e6c9b78adfc2..95af4a470f1e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -423,6 +423,21 @@ static __poll_t lineevent_poll(struct file *file,
 	return events;
 }
 
+static ssize_t lineevent_get_size(void)
+{
+#ifdef __x86_64__
+	/* i386 has no padding after 'id' */
+	if (in_ia32_syscall()) {
+		struct compat_gpioeevent_data {
+			compat_u64	timestamp;
+			u32		id;
+		};
+
+		return sizeof(struct compat_gpioeevent_data);
+	}
+#endif
+	return sizeof(struct gpioevent_data);
+}
 
 static ssize_t lineevent_read(struct file *file,
 			      char __user *buf,
@@ -432,9 +447,20 @@ static ssize_t lineevent_read(struct file *file,
 	struct lineevent_state *le = file->private_data;
 	struct gpioevent_data ge;
 	ssize_t bytes_read = 0;
+	ssize_t ge_size;
 	int ret;
 
-	if (count < sizeof(ge))
+	/*
+	 * When compatible system call is being used the struct gpioevent_data,
+	 * in case of at least ia32, has different size due to the alignment
+	 * differences. Because we have first member 64 bits followed by one of
+	 * 32 bits there is no gap between them. The only problematic is the
+	 * padding at the end of the data structure. Hence, we calculate the
+	 * actual sizeof() and pass this as an argument to copy_to_user() to
+	 * drop unneeded bytes from the output.
+	 */
+	ge_size = lineevent_get_size();
+	if (count < ge_size)
 		return -EINVAL;
 
 	do {
@@ -470,10 +496,10 @@ static ssize_t lineevent_read(struct file *file,
 			break;
 		}
 
-		if (copy_to_user(buf + bytes_read, &ge, sizeof(ge)))
+		if (copy_to_user(buf + bytes_read, &ge, ge_size))
 			return -EFAULT;
-		bytes_read += sizeof(ge);
-	} while (count >= bytes_read + sizeof(ge));
+		bytes_read += ge_size;
+	} while (count >= bytes_read + ge_size);
 
 	return bytes_read;
 }
-- 
2.28.0

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Arnd Bergmann]

On Mon, Sep 14, 2020 at 4:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The introduced line even handling ABI in the commit
>
>   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
>
> missed the fact that 64-bit kernel may serve for 32-bit applications.
> In such case the very first check in the lineevent_read() will fail
> due to alignment differences.
>
> To workaround this introduce lineeven_to_user() helper which returns actual
> size of the structure and copies its content to user if asked.
>
> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e6c9b78adfc2..95af4a470f1e 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -423,6 +423,21 @@ static __poll_t lineevent_poll(struct file *file,
>         return events;
>  }
>
> +static ssize_t lineevent_get_size(void)
> +{
> +#ifdef __x86_64__
> +       /* i386 has no padding after 'id' */
> +       if (in_ia32_syscall()) {

Christoph Hellwig has recently suggested adding a new macro for this
that would be always available and just evaluate to false on other
architectures.

I'd just merge your version for now and backport to to stable kernels,
but change this instance and a couple of others to use the new
macro in mainline afterwards.

Incidentally that would also address CONFIG_OABI_COMPAT
mode, if anyone cares.

      Arnd

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko]

On Mon, Sep 14, 2020 at 04:55:31PM +0200, Arnd Bergmann wrote:
> On Mon, Sep 14, 2020 at 4:37 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The introduced line even handling ABI in the commit
> >
> >   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> >
> > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > In such case the very first check in the lineevent_read() will fail
> > due to alignment differences.
> >
> > To workaround this introduce lineeven_to_user() helper which returns actual
> > size of the structure and copies its content to user if asked.
> >
> > Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index e6c9b78adfc2..95af4a470f1e 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -423,6 +423,21 @@ static __poll_t lineevent_poll(struct file *file,
> >         return events;
> >  }
> >
> > +static ssize_t lineevent_get_size(void)
> > +{
> > +#ifdef __x86_64__
> > +       /* i386 has no padding after 'id' */
> > +       if (in_ia32_syscall()) {
> 
> Christoph Hellwig has recently suggested adding a new macro for this
> that would be always available and just evaluate to false on other
> architectures.
> 
> I'd just merge your version for now and backport to to stable kernels,
> but change this instance and a couple of others to use the new
> macro in mainline afterwards.
> 
> Incidentally that would also address CONFIG_OABI_COMPAT
> mode, if anyone cares.

Good to know (for both items).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Kent Gibson]

On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:
> The introduced line even handling ABI in the commit
> 

s/even/event/

>   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> 
> missed the fact that 64-bit kernel may serve for 32-bit applications.
> In such case the very first check in the lineevent_read() will fail
> due to alignment differences.
> 
> To workaround this introduce lineeven_to_user() helper which returns actual
> size of the structure and copies its content to user if asked.
> 

s/lineeven_to_user/lineevent_get_size/

and

s/structure and copies its content to user if asked/structure in userspace/

> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: moved to just calculate size (Arnd, Kent), added good comment (Arnd)
>  drivers/gpio/gpiolib-cdev.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e6c9b78adfc2..95af4a470f1e 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -423,6 +423,21 @@ static __poll_t lineevent_poll(struct file *file,
>  	return events;
>  }
>  
> +static ssize_t lineevent_get_size(void)
> +{
> +#ifdef __x86_64__
> +	/* i386 has no padding after 'id' */
> +	if (in_ia32_syscall()) {
> +		struct compat_gpioeevent_data {
> +			compat_u64	timestamp;
> +			u32		id;
> +		};
> +
> +		return sizeof(struct compat_gpioeevent_data);
> +	}
> +#endif
> +	return sizeof(struct gpioevent_data);
> +}
>  

It can return size_t now.

>  static ssize_t lineevent_read(struct file *file,
>  			      char __user *buf,
> @@ -432,9 +447,20 @@ static ssize_t lineevent_read(struct file *file,
>  	struct lineevent_state *le = file->private_data;
>  	struct gpioevent_data ge;
>  	ssize_t bytes_read = 0;
> +	ssize_t ge_size;

Similarly here.

>  	int ret;
>  
> -	if (count < sizeof(ge))
> +	/*
> +	 * When compatible system call is being used the struct gpioevent_data,
> +	 * in case of at least ia32, has different size due to the alignment
> +	 * differences. Because we have first member 64 bits followed by one of
> +	 * 32 bits there is no gap between them. The only problematic is the
> +	 * padding at the end of the data structure. Hence, we calculate the
> +	 * actual sizeof() and pass this as an argument to copy_to_user() to
> +	 * drop unneeded bytes from the output.
> +	 */

s/problematic/difference/

> +	ge_size = lineevent_get_size();
> +	if (count < ge_size)
>  		return -EINVAL;
>  
>  	do {
> @@ -470,10 +496,10 @@ static ssize_t lineevent_read(struct file *file,
>  			break;
>  		}
>  
> -		if (copy_to_user(buf + bytes_read, &ge, sizeof(ge)))
> +		if (copy_to_user(buf + bytes_read, &ge, ge_size))
>  			return -EFAULT;
> -		bytes_read += sizeof(ge);
> -	} while (count >= bytes_read + sizeof(ge));
> +		bytes_read += ge_size;
> +	} while (count >= bytes_read + ge_size);
>  
>  	return bytes_read;
>  }
> -- 
> 2.28.0
> 

Cheers,
Kent.

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 07:05:26AM +0800, Kent Gibson wrote:

Thanks for your review. Before I'm going on it, can you confirm that these are
the only issues with the patch and after addressing them you will be okay with
the patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 07:05:26AM +0800, Kent Gibson wrote:
> On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:

...

> It can return size_t now.

> >  	ssize_t bytes_read = 0;
> > +	ssize_t ge_size;
> 
> Similarly here.

I deliberately left the ssize_t type to be consistent with the returned type of
the function and bytes_read. If you insist on the type change I will do, though
I personally like my approach.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Kent Gibson]

On Tue, Sep 15, 2020 at 12:20:22PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 15, 2020 at 07:05:26AM +0800, Kent Gibson wrote:
> > On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > It can return size_t now.
> 
> > >  	ssize_t bytes_read = 0;
> > > +	ssize_t ge_size;
> > 
> > Similarly here.
> 
> I deliberately left the ssize_t type to be consistent with the returned type of
> the function and bytes_read. If you insist on the type change I will do, though
> I personally like my approach.
> 

Bart prefers to use unsigned ints where variables are never negative,
and lineevent_get_size() never returns negative so should be size_t.
And it feels like a sizeof() to me so should return a size_t.

By the same logic bytes_read is never negative so it should be size_t as
well.  It seems reasonable to assume that bytes_read will always be less
than SSIZE_MAX so any cast to ssize_t for the return would be harmless.
Though changing that would probably mean a separate patch?

> Thanks for your review. Before I'm going on it, can you confirm that these are
> the only issues with the patch and after addressing them you will be okay with
> the patch?

I have suggested renaming ge_size to event_size, but that is just personal
preference. You have more than enough documentation describing the issue
where it is assigned, so I'm fine with that.

These are just my suggestions. Feel free to ignore them.

Cheers,
Kent.

Re: [PATCH v2] gpiolib: Fix line event handling in syscall compatible mode

[Andy Shevchenko]

On Tue, Sep 15, 2020 at 08:18:15PM +0800, Kent Gibson wrote:
> On Tue, Sep 15, 2020 at 12:20:22PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 15, 2020 at 07:05:26AM +0800, Kent Gibson wrote:
> > > On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > It can return size_t now.
> > 
> > > >  	ssize_t bytes_read = 0;
> > > > +	ssize_t ge_size;
> > > 
> > > Similarly here.
> > 
> > I deliberately left the ssize_t type to be consistent with the returned type of
> > the function and bytes_read. If you insist on the type change I will do, though
> > I personally like my approach.
> > 
> 
> Bart prefers to use unsigned ints where variables are never negative,
> and lineevent_get_size() never returns negative so should be size_t.
> And it feels like a sizeof() to me so should return a size_t.
> 
> By the same logic bytes_read is never negative so it should be size_t as
> well.  It seems reasonable to assume that bytes_read will always be less
> than SSIZE_MAX so any cast to ssize_t for the return would be harmless.
> Though changing that would probably mean a separate patch?
> 
> > Thanks for your review. Before I'm going on it, can you confirm that these are
> > the only issues with the patch and after addressing them you will be okay with
> > the patch?
> 
> I have suggested renaming ge_size to event_size, but that is just personal
> preference. You have more than enough documentation describing the issue
> where it is assigned, so I'm fine with that.
> 
> These are just my suggestions. Feel free to ignore them.

Thanks for review!

I will send v3 soon, but I will leave ssize_t by the reasons I mentioned above.

-- 
With Best Regards,
Andy Shevchenko