Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Sep 10, 2020 at 01:19:34PM +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.
> 

And again here.

> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Suggested-by: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-cdev.c | 41 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e6c9b78adfc2..a6a8384c8255 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -423,6 +423,33 @@ static __poll_t lineevent_poll(struct file *file,
>  	return events;
>  }
>  
> +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge)
> +{
> +#ifdef __x86_64__
> +	/* i386 has no padding after 'id' */
> +	if (in_ia32_syscall()) {
> +		struct compat_ge {
> +			compat_u64	timestamp __packed;
> +			u32		id;
> +		} cge;
> +
> +		if (buf && ge) {
> +			cge = (struct compat_ge){ ge->timestamp, ge->id };
> +			if (copy_to_user(buf, &cge, sizeof(cge)))
> +				return -EFAULT;
> +		}
> +
> +		return sizeof(cge);
> +	}
> +#endif
> +
> +	if (buf && ge) {
> +		if (copy_to_user(buf, ge, sizeof(*ge)))
> +			return -EFAULT;
> +	}
> +
> +	return sizeof(*ge);
> +}
>  

The dual-purpose nature of this function makes it more complicated than
it needs to be.
I was going to suggest splitting it into separate functions, but...

Given that struct compat_ge is a strict truncation of struct
gpioevent_data, how about reducing this function to just determining the
size of the event for user space, say lineevent_user_size(), and
replacing sizeof(ge) with that calculcated size throughout
lineevent_read()?

>  static ssize_t lineevent_read(struct file *file,
>  			      char __user *buf,
> @@ -432,9 +459,12 @@ 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 argument is NULL it returns size of the structure in user space */
> +	ge_size = lineevent_to_user(NULL, NULL);
> +	if (count < ge_size)
>  		return -EINVAL;
>  
>  	do {
> @@ -470,10 +500,11 @@ static ssize_t lineevent_read(struct file *file,
>  			break;
>  		}
>  
> -		if (copy_to_user(buf + bytes_read, &ge, sizeof(ge)))
> -			return -EFAULT;
> -		bytes_read += sizeof(ge);
> -	} while (count >= bytes_read + sizeof(ge));
> +		ret = lineevent_to_user(buf + bytes_read, &ge);
> +		if (ret < 0)
> +			return ret;
> +		bytes_read += ret;
> +	} while (count >= bytes_read + ge_size);
>  
>  	return bytes_read;
>  }
> -- 
> 2.28.0
> 

Is patch 2 in any way related to this patch?

Cheers,
Kent.