linux-kernel - [PATCH v2 3/3] gpiolib: cdev: Cleanup kfifo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240529131953.195777-4-warthog618@gmail.com>
Date: Wed, 29 May 2024 21:19:53 +0800
From: Kent Gibson <warthog618@...il.com>
To: linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org,
	brgl@...ev.pl,
	linus.walleij@...aro.org
Cc: Kent Gibson <warthog618@...il.com>
Subject: [PATCH v2 3/3] gpiolib: cdev: Cleanup kfifo_out() error handling

The handling of kfifo_out() errors in read functions obscures any error.
The error condition should never occur but, while a ret is set to -EIO, it
is subsequently ignored and the read functions instead return the number
of bytes copied to that point, potentially masking the fact that any error
occurred.

Log a warning and return -EIO in the case of a kfifo_out() error to make
it clear something very odd is going on here.

Signed-off-by: Kent Gibson <warthog618@...il.com>
---
 drivers/gpio/gpiolib-cdev.c | 53 +++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index c7218c9f2c5e..1cb952daacfb 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1642,16 +1642,15 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 					return ret;
 			}

-			ret = kfifo_out(&lr->events, &le, 1);
-		}
-		if (ret != 1) {
-			/*
-			 * This should never happen - we were holding the
-			 * lock from the moment we learned the fifo is no
-			 * longer empty until now.
-			 */
-			ret = -EIO;
-			break;
+			if (kfifo_out(&lr->events, &le, 1) != 1) {
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				WARN(1, "failed to read from non-empty kfifo");
+				return -EIO;
+			}
 		}

 		if (copy_to_user(buf + bytes_read, &le, sizeof(le)))
@@ -1995,16 +1994,15 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 					return ret;
 			}

-			ret = kfifo_out(&le->events, &ge, 1);
-		}
-		if (ret != 1) {
-			/*
-			 * This should never happen - we were holding the lock
-			 * from the moment we learned the fifo is no longer
-			 * empty until now.
-			 */
-			ret = -EIO;
-			break;
+			if (kfifo_out(&le->events, &ge, 1) != 1) {
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				WARN(1, "failed to read from non-empty kfifo");
+				return -EIO;
+			}
 		}

 		if (copy_to_user(buf + bytes_read, &ge, ge_size))
@@ -2707,12 +2705,15 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 			if (count < event_size)
 				return -EINVAL;
 #endif
-			ret = kfifo_out(&cdev->events, &event, 1);
-		}
-		if (ret != 1) {
-			ret = -EIO;
-			break;
-			/* We should never get here. See lineevent_read(). */
+			if (kfifo_out(&cdev->events, &event, 1) != 1) {
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				WARN(1, "failed to read from non-empty kfifo");
+				return -EIO;
+			}
 		}

 #ifdef CONFIG_GPIO_CDEV_V1
--
2.39.2