Re: [PATCH] tools: gpio: fix buffer overflow and add bounds check

["Maxwell Doose" <m32285159@gmail.com>]

On Sun May 3, 2026 at 2:00 PM CDT, Zhang Xiaolei wrote:
> Replace strcpy() with strncpy() to avoid potential buffer overflow
> in req.consumer. Also add validation for num_lines to prevent
> out-of-bounds access to req.offsets.
>
> Fix incorrect ioctl name in error message.
>
> Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
> ---
>  tools/gpio/gpio-utils.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>

This looks like a patch that could be split, since you're adding
NULL pointer checking, a potential overflow fix, and a style fix all
into one. I like the idea, but we need to split functional changes.

>
> diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
> index 4096bcd511d1..1afd9dff2bed 100644
> --- a/tools/gpio/gpio-utils.c
> +++ b/tools/gpio/gpio-utils.c
> @@ -65,11 +65,15 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
>  	int i;
>  	int ret;
>  
> +	if (!device_name || !lines || !config || !consumer ||
> +	    num_lines == 0 || num_lines > GPIO_V2_LINES_MAX)
> +		return -EINVAL;
> +
>

First off, this can be split into a different patch. Secondly, I feel as
if we should make two if statements for this, one for the NULL and 0
checking and the other for checking num_lines. And returning -EINVAL
will be ambiguous for the caller, so maybe change that.

>
>  	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
>  	if (ret < 0)
>  		return -ENOMEM;
>  
> -	fd = open(chrdev_name, 0);
> +	fd = open(chrdev_name, O_RDONLY);
>

Another thing that can be split.

>
>  	if (fd == -1) {
>  		ret = -errno;
>  		fprintf(stderr, "Failed to open %s, %s\n",
> @@ -78,27 +82,29 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
>  	}
>  
>  	memset(&req, 0, sizeof(req));
> +
>

This seems like a stray change.

>
>  	for (i = 0; i < num_lines; i++)
>  		req.offsets[i] = lines[i];
>  
>  	req.config = *config;
> -	strcpy(req.consumer, consumer);
> +	strncpy(req.consumer, consumer, sizeof(req.consumer) - 1);
> +	req.consumer[sizeof(req.consumer) - 1] = '\0';
>

We already invented a solution for this in the form of strscpy(), so
please change this to use that instead, something like:

	strscpy(req.consumer, consumer, sizeof(req.consumer));

>
>  	req.num_lines = num_lines;
>  
>  	ret = ioctl(fd, GPIO_V2_GET_LINE_IOCTL, &req);
>  	if (ret == -1) {
>  		ret = -errno;
>  		fprintf(stderr, "Failed to issue %s (%d), %s\n",
> -			"GPIO_GET_LINE_IOCTL", ret, strerror(errno));
> +			"GPIO_V2_GET_LINE_IOCTL", ret, strerror(errno));
>

Another thing that could be split.

>
>  	}
>  
>  	if (close(fd) == -1)
>  		perror("Failed to close GPIO character device file");
> +
>

Might be another stray change.

>
>  exit_free_name:
>  	free(chrdev_name);
>  	return ret < 0 ? ret : req.fd;
>  }
> -
>

Maybe another stray change?

>
>  /**
>   * gpiotools_set_values() - Set the value of gpio(s)
>   * @fd:			The fd returned by

I like the idea, but we should be splitting some of these changes to
follow the atomic commits idea of the kernel.

best regards,
maxwell