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

[Kent Gibson <warthog618@xxxxxxxxx>]

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@xxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> 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.