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

* [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
@ 2019-12-04 19:42 Andy Shevchenko
  2019-12-04 19:42 ` [PATCH v1 2/2] gpiolib: Make use of assign_bit() API Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andy Shevchenko @ 2019-12-04 19:42 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio; +Cc: 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.

To workaround this we do several things here:
- put warning comment to UAPI header near to the structure description
- derive the size of the structure in the compatible mode from its members
- check for the size of this structure in the ->read() callback
- return only one event in the compatible mode at a time

Above mitigation will work at least with libgpiod which does one event
at a time.

Since the bug hasn't been reported earlier we assume that there is close
to zero actual users of the compatible mode to monitor GPIO events and thus
we might consider to rework this ABI in the future.

Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/gpio.h |  6 +++++
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7340e4d0e873..134985210619 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -825,17 +825,26 @@ static __poll_t lineevent_poll(struct file *filep,
 	return events;
 }
 
-
 static ssize_t lineevent_read(struct file *filep,
 			      char __user *buf,
 			      size_t count,
 			      loff_t *f_ps)
 {
 	struct lineevent_state *le = filep->private_data;
+	struct gpioevent_data event, *e = &event;
+	/* We need size of each member to avoid endianess issues below */
+	size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
 	unsigned int copied;
 	int ret;
 
-	if (count < sizeof(struct gpioevent_data))
+	/*
+	 * In compatible mode, when kernel is 64-bit and user space is 32-bit,
+	 * we may not tell what user wanted here when count is bigger than size
+	 * of one event, so, we just assume that user asks for precisely one
+	 * event.
+	 */
+	e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
+	if (count < e_sz)
 		return -EINVAL;
 
 	do {
@@ -851,7 +860,43 @@ static ssize_t lineevent_read(struct file *filep,
 
 		if (mutex_lock_interruptible(&le->read_lock))
 			return -ERESTARTSYS;
-		ret = kfifo_to_user(&le->events, buf, count, &copied);
+		if (in_compat_syscall()) {
+			/*
+			 * First we peek the one event and, if there is
+			 * no error during copying to user space, skip it
+			 * later.
+			 */
+			if (kfifo_peek(&le->events, e))
+				copied = e_sz;
+			else
+				copied = 0;
+
+			/* Do not try to copy garbage to the user */
+			ret = copied ? 0 : -EFAULT;
+
+			/*
+			 * Due to endianess concerns we have to copy this
+			 * member-by-member. Luckily there are no members
+			 * less than 32-bit.
+			 */
+			if (!ret)
+				ret = copy_to_user(buf, &e->timestamp, ts_sz);
+			if (!ret)
+				ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
+
+			if (ret) {
+				/*
+				 * Either we have got nothing from the FIFO or
+				 * one of copy_to_user() calls failed.
+				 */
+				ret = -EFAULT;
+			} else {
+				/* Skip peeked event if no error happened */
+				kfifo_skip(&le->events);
+			}
+		} else {
+			ret = kfifo_to_user(&le->events, buf, count, &copied);
+		}
 		mutex_unlock(&le->read_lock);
 
 		if (ret)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 799cf823d493..054756bf6991 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -168,6 +168,12 @@ struct gpioevent_request {
  * struct gpioevent_data - The actual event being pushed to userspace
  * @timestamp: best estimate of time of event occurrence, in nanoseconds
  * @id: event identifier
+ *
+ * Warning! This structure has issues in the compatible mode, when
+ * kernel is 64-bit and user space is 32-bit, due to alignment
+ * differences.
+ *
+ * It's not recommended to retrieve more than one event at a time.
  */
 struct gpioevent_data {
 	__u64 timestamp;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread