gpiolib: Fix line event handling in syscall compatible mode

Use DEFINE_SEQ_ATTRIBUTE macro to simplify the code.

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

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 <[email protected]>

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

---
 drivers/gpio/gpiolib-cdev.c | 41 ++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

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

index e6c9b78adfc2..a6a8384c8255 100644
@@ -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);
+	}

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

 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;
 }

Signature · 1 line

2.28.0

Quoted 2 lines · Andy Shevchenko

The introduced line even handling ABI in the commit

s/even/event/

Quoted 6 lines

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 3 lines

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.

Quoted 16 lines

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

+#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);

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

Quoted 10 lines

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

+	}
+#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()?

Is patch 2 in any way related to this patch?

Quoted 12 lines · Kent Gibson

Quoted 2 lines · Andy Shevchenko

The introduced line even handling ABI in the commit

s/even/event/

Quoted 6 lines

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 5 lines

Quoted 3 lines

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.

Thanks!

...

Quoted 31 lines

Quoted 16 lines

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

+#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);

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

Quoted 10 lines

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

+	}
+#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...

Quoted 5 lines

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

So you mean something like

	struct compat_gpioeevent_data {
		compat_u64	timestamp;
		u32		id;
	} __packed;

#ifdef __x86_64__
	/* i386 has no padding after 'id' */
	size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data);
#else
	size_t ge_size = sizeof(struct gpioevent_data) ;

#endif

?

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:

Quoted 43 lines

Quoted 30 lines

Quoted 15 lines

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

+#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);

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

Quoted 10 lines

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

+	}
+#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...

Quoted 5 lines

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

So you mean something like

	struct compat_gpioeevent_data {
		compat_u64	timestamp;
		u32		id;
	} __packed;

Quoted 9 lines

#ifdef __x86_64__
	/* i386 has no padding after 'id' */
	size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data);
#else
	size_t ge_size = sizeof(struct gpioevent_data) ;

#endif

?

Pretty much, though I was suggesting keeping it in a helper function, say lineevent_user_size(), not in lineevent_read() itself, to isolate all the ugliness in one small place.

So in lineevent_read() you would:

   size_t ge_size = lineevent_user_size();

and then use that to replace all the sizeof(ge) occurrences.

Quoted 3 lines · Kent Gibson

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:

...

Quoted 57 lines

Quoted 43 lines

Quoted 30 lines

Quoted 15 lines

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

+#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);

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

Quoted 10 lines

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

+	}
+#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...

Quoted 5 lines

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

So you mean something like

	struct compat_gpioeevent_data {
		compat_u64	timestamp;
		u32		id;
	} __packed;

Quoted 9 lines

#ifdef __x86_64__
	/* i386 has no padding after 'id' */
	size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data);
#else
	size_t ge_size = sizeof(struct gpioevent_data) ;

#endif

?

Pretty much, though I was suggesting keeping it in a helper function, say lineevent_user_size(), not in lineevent_read() itself, to isolate all the ugliness in one small place.

Quoted 5 lines

So in lineevent_read() you would:

   size_t ge_size = lineevent_user_size();

and then use that to replace all the sizeof(ge) occurrences.

But in any case it makes code a bit hackish, no?

We calculate the size of one structure and by the fact *partially* copy another one.

And actually if you look closer to the types, the compat_u64 is not the same as u64. So, I'm not sure your solution would work in all cases.

Quoted 6 lines · Kent Gibson

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Kent Gibson

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:

Quoted 4 lines

typedef u64 __attribute__((aligned(4))) compat_u64;

which is bitwise identical - only allowed to 32-bit align.

Yes. That's what I meant under "not the same".

As far as I understand the alignment makes sense if this type is a part of the uAPI definition. But here we have it completely local. copy_to_user() takes a pointer to a memory without any specific alignment implied.

So, what you proposing is basically something like

ret = copy_to_user(buf, &ge, compat ? sizeof(compat) : sizeof(ge));

Correct?

I don't like the difference between 2nd and 3rd argument. This what looks to me hackish. Variant with explicit compat structure I like more.

But if you think it's okay, I will update your way.

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Kent Gibson

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Kent Gibson

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Kent Gibson

On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:

[snip]

Quoted 11 lines

Quoted 4 lines

typedef u64 __attribute__((aligned(4))) compat_u64;

which is bitwise identical - only allowed to 32-bit align.

Yes. That's what I meant under "not the same".

As far as I understand the alignment makes sense if this type is a part of the uAPI definition. But here we have it completely local. copy_to_user() takes a pointer to a memory without any specific alignment implied.

Quoted 6 lines

So, what you proposing is basically something like

ret = copy_to_user(buf, &ge, compat ? sizeof(compat) : sizeof(ge));

Correct?

That isn't how I would write the copy_to_user(). The size would be calculated once, using the linevent_user_size() helper, with appropriate documentation as to why this is necessary, and then used throughout lineevent_read().

The documentation would mainly be on the lineevent_user_size() function itself.

Quoted 3 lines

I don't like the difference between 2nd and 3rd argument. This what looks to me hackish. Variant with explicit compat structure I like more.

Agreed - writing it that way does look pretty nasty.

But my suggestion is actually this:

ret = copy_to_user(buf, &ge, event_size);

I suggested ge_size previously, but event_size might help highlight that it isn't always sizeof(ge).

Quoted 2 lines

But if you think it's okay, I will update your way.

I would defer to Bart or Linus, but I think just calculating the appropriate size is preferable for this case.

Cheers,
Kent.

Quoted 2 lines · Kent Gibson

Quoted 1 line · Andy Shevchenko

On Fri, Sep 11, 2020 at 06:17:14PM +0800, Kent Gibson wrote:

Quoted 19 lines

Quoted 3 lines

I don't like the difference between 2nd and 3rd argument. This what looks to me hackish. Variant with explicit compat structure I like more.

Agreed - writing it that way does look pretty nasty.

But my suggestion is actually this:

ret = copy_to_user(buf, &ge, event_size);

I suggested ge_size previously, but event_size might help highlight that it isn't always sizeof(ge).

Quoted 2 lines

But if you think it's okay, I will update your way.

I would defer to Bart or Linus, but I think just calculating the appropriate size is preferable for this case.

Quoted 2 lines

Cheers,
Kent.

Kent has been producing clean and elegant code so far so I trust him on code quality issues TBH. Personally in this case however I'd go with an explicit compat structure as Andy prefers.

I don't have a strong opinion on this so I really am fine either way.

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

Quoted 3 lines

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

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

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.

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

Quoted 4 lines

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

+       /* 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.

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

Quoted 4 lines

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

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

drivers/gpio/gpiolib-cdev.c static ssize_t lineevent_read(struct file *file, · line 468

Quoted 5 lines

drivers/gpio/gpiolib-cdev.c +3 -1

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