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

[007 <zxl434815272@gmail.com>]

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