| Message ID | 20200914143743.39871-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 5ad284ab3a01e2d6a89be2a8663ae76f4e617549 |
| Headers | show |
| Series | [v2] gpiolib: Fix line event handling in syscall compatible mode | expand |
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
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
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.
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?
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
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.
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
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; }
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(-)