| Message ID | 20200910101935.47140-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] gpiolib: Fix line event handling in syscall compatible mode | expand |
On Fri, Sep 11, 2020 at 06:20:49PM +0200, Arnd Bergmann wrote: > > +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge) > > +{ > > +#ifdef __x86_64__ > > I would make this "#ifdef CONFIG_IA32_COMPAT" to clarify what this > is actually checking for. There is no such option available right now, I prefer to leave as is to make backporting easier. > In theory we could add support for > CONFIG_OABI_COMPAT here as well, not sure if there is a point. > I recently came across a couple of things that all need the same > hacks for arm-oabi and x86-32 in principle. > > > + /* i386 has no padding after 'id' */ > > + if (in_ia32_syscall()) { > > + struct compat_ge { > > + compat_u64 timestamp __packed; > > No need for marking this __packed, it already is. Yeah, due to a special alignment for compat_u64. I blindly copied from your proposal. > > + u32 id; > > + } cge; > > + > > + if (buf && ge) { > > I think I'd leave out the 'if()' checks here, and require the function > to be called with valid pointers. It seems odd otherwise to return > sizeof(cge) from the read() function without having written data. > > Note also that user space may pass a NULL pointer and should > get back -EFAULT instead of 12 or 16. OK! > > - 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; > > Right, I see this is how it's being used, and I'd tend to agree with Kent: > if you just determine the size dynamically and add a good comment, > then the rest of the code gets simpler and more logical. Okay, I will re-do this. -- With Best Regards, Andy Shevchenko
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); +} 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; }
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> --- drivers/gpio/gpiolib-cdev.c | 41 ++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-)