Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-gpio@vger.kernel.org
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode
Date: Wed,  4 Dec 2019 21:42:28 +0200
Message-ID: <20191204194229.64251-1-andriy.shevchenko@linux.intel.com> (raw)

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


             reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-04 19:42 Andy Shevchenko [this message]
2019-12-04 19:42 ` [PATCH v1 2/2] gpiolib: Make use of assign_bit() API Andy Shevchenko
2019-12-06 10:57 ` [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode Bartosz Golaszewski
2019-12-10  9:06 ` Bartosz Golaszewski
2019-12-10 13:44   ` Andy Shevchenko
2019-12-10 14:39   ` Kent Gibson
2019-12-10 16:55     ` Andy Shevchenko

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191204194229.64251-1-andriy.shevchenko@linux.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git