core: check for positive values returned by calls to ioctl()

PATCH In review Bartosz Golaszewski 2 participants 2 messages Jan 25, 2024

Subsystems (2)

Library code GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2024-01-25 08:06 UTC #0 lore raw

PATCH v2 1/1 core: check for positive values returned by calls to ioctl()

From: Bartosz Golaszewski <[email protected]>

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

Fixes b7ba732e6a93 ("treewide: libgpiod v2 implementation")

Signed-off-by Bartosz Golaszewski <[email protected]>

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

lib/chip.c +8 -9

index 7c05e53..611eb32 100644
@@ -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);

lib/internal.c +13 -0

index c80d01f..56cb8b9 100644
@@ -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;

lib/internal.h +1 -0

index 61d7aec..420fbdd 100644
@@ -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);

lib/line-request.c +6 -5

index e867d91..b76b3d7 100644
@@ -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;

Signature · 1 line

2.40.1

Kent Gibson

Reviewed-by on v2 1/1

2024-01-25 12:45 UTC #1 lore raw

Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 6 lines

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 <[email protected]>
Fixes b7ba732e6a93 ("treewide: libgpiod v2 implementation")
Signed-off-by Bartosz Golaszewski <[email protected]>

Reviewed-by Kent Gibson <[email protected]>