[libgpiod][PATCH v2] core: check for positive values returned by calls to ioctl()

[libgpiod][PATCH v2] core: check for positive values returned by calls to ioctl()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If the kernel GPIO driver (erroneously) returns a positive value from one
of its callbacks, it may end up being propagated to user space as
a positive value returned by the call to ioctl(). Let's treat all
non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever
return positive values.

To that end let's create a wrapper around the libc's ioctl() that checks
the return value and sets errno to EBADE (Invalid exchange) if it's
greater than 0.

This should be addressed in the kernel but will remain a problem on older
or unpatched versions so we need to sanitize it in user-space too.

Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com>
Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v1 -> v2:
- add a wrapper around ioctl() that sets errno to EBADE in case of
  a positive return value

 lib/chip.c         | 17 ++++++++---------
 lib/internal.c     | 13 +++++++++++++
 lib/internal.h     |  1 +
 lib/line-request.c | 11 ++++++-----
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/lib/chip.c b/lib/chip.c
index 7c05e53..611eb32 100644
--- a/lib/chip.c
+++ b/lib/chip.c
@@ -7,7 +7,6 @@
 #include <gpiod.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/ioctl.h>
 #include <unistd.h>
 
 #include "internal.h"
@@ -72,7 +71,7 @@ static int read_chip_info(int fd, struct gpiochip_info *info)
 
 	memset(info, 0, sizeof(*info));
 
-	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, info);
+	ret = gpiod_ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, info);
 	if (ret)
 		return -1;
 
@@ -87,7 +86,7 @@ GPIOD_API struct gpiod_chip_info *gpiod_chip_get_info(struct gpiod_chip *chip)
 	assert(chip);
 
 	ret = read_chip_info(chip->fd, &info);
-	if (ret < 0)
+	if (ret)
 		return NULL;
 
 	return gpiod_chip_info_from_uapi(&info);
@@ -111,7 +110,7 @@ static int chip_read_line_info(int fd, unsigned int offset,
 	cmd = watch ? GPIO_V2_GET_LINEINFO_WATCH_IOCTL :
 		      GPIO_V2_GET_LINEINFO_IOCTL;
 
-	ret = ioctl(fd, cmd, info);
+	ret = gpiod_ioctl(fd, cmd, info);
 	if (ret)
 		return -1;
 
@@ -150,7 +149,7 @@ GPIOD_API int gpiod_chip_unwatch_line_info(struct gpiod_chip *chip,
 {
 	assert(chip);
 
-	return ioctl(chip->fd, GPIO_GET_LINEINFO_UNWATCH_IOCTL, &offset);
+	return gpiod_ioctl(chip->fd, GPIO_GET_LINEINFO_UNWATCH_IOCTL, &offset);
 }
 
 GPIOD_API int gpiod_chip_get_fd(struct gpiod_chip *chip)
@@ -192,7 +191,7 @@ GPIOD_API int gpiod_chip_get_line_offset_from_name(struct gpiod_chip *chip,
 	}
 
 	ret = read_chip_info(chip->fd, &chinfo);
-	if (ret < 0)
+	if (ret)
 		return -1;
 
 	for (offset = 0; offset < chinfo.lines; offset++) {
@@ -235,11 +234,11 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,
 		return NULL;
 
 	ret = read_chip_info(chip->fd, &info);
-	if (ret < 0)
+	if (ret)
 		return NULL;
 
-	ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req);
-	if (ret < 0)
+	ret = gpiod_ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req);
+	if (ret)
 		return NULL;
 
 	request = gpiod_line_request_from_uapi(&uapi_req, info.name);
diff --git a/lib/internal.c b/lib/internal.c
index c80d01f..56cb8b9 100644
--- a/lib/internal.c
+++ b/lib/internal.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
 #include <sys/types.h>
@@ -121,6 +122,18 @@ int gpiod_set_output_value(enum gpiod_line_value in, enum gpiod_line_value *out)
 	return 0;
 }
 
+int gpiod_ioctl(int fd, unsigned long request, void *arg)
+{
+	int ret;
+
+	ret = ioctl(fd, request, arg);
+	if (ret <= 0)
+		return ret;
+
+	errno = EBADE;
+	return -1;
+}
+
 void gpiod_line_mask_zero(uint64_t *mask)
 {
 	*mask = 0ULL;
diff --git a/lib/internal.h b/lib/internal.h
index 61d7aec..420fbdd 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -38,6 +38,7 @@ struct gpiod_info_event *gpiod_info_event_read_fd(int fd);
 int gpiod_poll_fd(int fd, int64_t timeout);
 int gpiod_set_output_value(enum gpiod_line_value in,
 			   enum gpiod_line_value *out);
+int gpiod_ioctl(int fd, unsigned long request, void *arg);
 
 void gpiod_line_mask_zero(uint64_t *mask);
 bool gpiod_line_mask_test_bit(const uint64_t *mask, int nr);
diff --git a/lib/line-request.c b/lib/line-request.c
index e867d91..b76b3d7 100644
--- a/lib/line-request.c
+++ b/lib/line-request.c
@@ -6,7 +6,6 @@
 #include <gpiod.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/ioctl.h>
 #include <sys/param.h>
 #include <unistd.h>
 
@@ -153,7 +152,8 @@ gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
 
 	uapi_values.mask = mask;
 
-	ret = ioctl(request->fd, GPIO_V2_LINE_GET_VALUES_IOCTL, &uapi_values);
+	ret = gpiod_ioctl(request->fd, GPIO_V2_LINE_GET_VALUES_IOCTL,
+			  &uapi_values);
 	if (ret)
 		return -1;
 
@@ -218,7 +218,8 @@ gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
 	uapi_values.mask = mask;
 	uapi_values.bits = bits;
 
-	return ioctl(request->fd, GPIO_V2_LINE_SET_VALUES_IOCTL, &uapi_values);
+	return gpiod_ioctl(request->fd, GPIO_V2_LINE_SET_VALUES_IOCTL,
+			   &uapi_values);
 }
 
 GPIOD_API int gpiod_line_request_set_values(struct gpiod_line_request *request,
@@ -271,8 +272,8 @@ gpiod_line_request_reconfigure_lines(struct gpiod_line_request *request,
 		return -1;
 	}
 
-	ret = ioctl(request->fd, GPIO_V2_LINE_SET_CONFIG_IOCTL,
-		    &uapi_cfg.config);
+	ret = gpiod_ioctl(request->fd, GPIO_V2_LINE_SET_CONFIG_IOCTL,
+			  &uapi_cfg.config);
 	if (ret)
 		return ret;
 
-- 
2.40.1

Re: [libgpiod][PATCH v2] core: check for positive values returned by calls to ioctl()

[Kent Gibson]

On Thu, Jan 25, 2024 at 09:06:29AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> If the kernel GPIO driver (erroneously) returns a positive value from one
> of its callbacks, it may end up being propagated to user space as
> a positive value returned by the call to ioctl(). Let's treat all
> non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever
> return positive values.
>
> To that end let's create a wrapper around the libc's ioctl() that checks
> the return value and sets errno to EBADE (Invalid exchange) if it's
> greater than 0.
>
> This should be addressed in the kernel but will remain a problem on older
> or unpatched versions so we need to sanitize it in user-space too.
>
> Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com>
> Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Kent Gibson <warthog618@gmail.com>