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

Kent Gibson
2020-09-14T23:05:35Z

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.