gpiolib: Fix line event handling in syscall compatible mode

PATCH In review Andy Shevchenko 3 participants 8 messages Sep 14, 2020 – Sep 15, 2020

Subsystems (2)

GPIO GPIO uapi

lore

Share & tools

lore

Andy Shevchenko

author

2020-09-14 14:38 UTC #0 lore raw

PATCH v2 1/1 gpiolib: Fix line event handling in syscall compatible mode

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")

Signed-off-by Andy Shevchenko <[email protected]>

---
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(-)

drivers/gpio/gpiolib-cdev.c +30 -4

index e6c9b78adfc2..95af4a470f1e 100644
@@ -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;
 }

Signature · 1 line

2.28.0

Arnd Bergmann

Acked-by on v2 1/1

2020-09-14 14:55 UTC #1 lore raw

Quoted 9 lines · Andy Shevchenko

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.

Quoted 6 lines

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 <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

drivers/gpio/gpiolib-cdev.c static __poll_t lineevent_poll(struct file *file, · line 430

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c +5 -0

index e6c9b78adfc2..95af4a470f1e 100644
@@ -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

Andy Shevchenko

author

2020-09-14 15:02 UTC #2 lore raw

Quoted 11 lines · Arnd Bergmann

Quoted 9 lines · Andy Shevchenko
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.

Quoted 8 lines

Quoted 6 lines
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 <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
Acked-by Arnd Bergmann <[email protected]>

Thanks!

Quoted 18 lines

drivers/gpio/gpiolib-cdev.c static __poll_t lineevent_poll(struct file *file, · line 430
Quoted 13 lines
drivers/gpio/gpiolib-cdev.c +5 -0
index e6c9b78adfc2..95af4a470f1e 100644
@@ -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.

Quoted 6 lines

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).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2020-09-15 09:14 UTC #4 lore raw

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?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2020-09-15 09:24 UTC #5 lore raw

In reply to Kent Gibson

Quoted 1 line · Kent Gibson

On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:

...

Quoted 6 lines

It can return size_t now.
drivers/gpio/gpiolib-cdev.c static ssize_t lineevent_read(struct file *file, · line 450
Quoted 2 lines
drivers/gpio/gpiolib-cdev.c +1 -0
 	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!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Kent Gibson

reviewer

2020-09-15 12:20 UTC #6 lore raw

Quoted 7 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson
On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:
...
Quoted 1 line
It can return size_t now.

Quoted 9 lines

Quoted 4 lines
drivers/gpio/gpiolib-cdev.c static ssize_t lineevent_read(struct file *file, · line 450
Quoted 2 lines
drivers/gpio/gpiolib-cdev.c +1 -0
 	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?

Quoted 3 lines

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.

Andy Shevchenko

author

2020-09-15 12:59 UTC #7 lore raw

Quoted 31 lines · Kent Gibson

Quoted 7 lines · Andy Shevchenko
Quoted 1 line · Kent Gibson
On Mon, Sep 14, 2020 at 05:37:43PM +0300, Andy Shevchenko wrote:
...
Quoted 1 line
It can return size_t now.
Quoted 9 lines
Quoted 4 lines
drivers/gpio/gpiolib-cdev.c static ssize_t lineevent_read(struct file *file, · line 450
Quoted 2 lines
drivers/gpio/gpiolib-cdev.c +1 -0
 	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?
Quoted 3 lines
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?

Quoted 5 lines

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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko