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

[007]

Hi Maxwell,

> 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.

Thanks for the review. I agree that these changes should be split into
smaller atomic patches. I will prepare a v2 series.

> 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.

Got it. I will split the argument validation and the num_lines bounds
check, and reconsider the return values.

> Another thing that can be split.

I will drop the O_RDONLY change from v2 unless it is useful as a separate
cleanup.

> This seems like a stray change.

Thanks, I will remove unrelated whitespace changes.

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

Agreed. I will use strscpy() in v2.

> Another thing that could be split.

I will split the ioctl name fix into a separate patch.

Thanks,
Zhang Xiaolei

On 5/4/26 04:56, Maxwell Doose wrote:
> 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@xxxxxxxxx>
> > ---
> >   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