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

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

[Zhang Xiaolei]

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

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;
+
 	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
 	if (ret < 0)
 		return -ENOMEM;
 
-	fd = open(chrdev_name, 0);
+	fd = open(chrdev_name, O_RDONLY);
 	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));
+
 	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';
 	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));
 	}
 
 	if (close(fd) == -1)
 		perror("Failed to close GPIO character device file");
+
 exit_free_name:
 	free(chrdev_name);
 	return ret < 0 ? ret : req.fd;
 }
-
 /**
  * gpiotools_set_values() - Set the value of gpio(s)
  * @fd:			The fd returned by
-- 
2.54.0

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

[Maxwell Doose]

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

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

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

[PATCH v2 1/3] tools: gpio: use strscpy() for consumer name

[Zhang Xiaolei]

Replace strcpy() with strscpy() to avoid potential buffer overflow
when copying the consumer string.

Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
---
 tools/gpio/gpio-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 4096bcd511d1..176bccfcccb0 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -82,7 +82,7 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
 		req.offsets[i] = lines[i];
 
 	req.config = *config;
-	strcpy(req.consumer, consumer);
+	strcpy(req.consumer, consumer, sizeof(req.consumer));
 	req.num_lines = num_lines;
 
 	ret = ioctl(fd, GPIO_V2_GET_LINE_IOCTL, &req);
-- 
2.34.1

[PATCH v2 2/3] tools: gpio: validate arguments in gpiotools_request_line

[Zhang Xiaolei]

Add validation for input pointers and number of lines.

Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
---
 tools/gpio/gpio-utils.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 176bccfcccb0..930a38fe7911 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -65,6 +65,12 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
 	int i;
 	int ret;
 
+	if (!device_name || !lines || !config || !consumer || !num_lines)
+		return -EINVAL;
+
+	if (num_lines > GPIO_V2_LINES_MAX)
+		return -EINVAL;
+
 	ret = asprintf(&chrdev_name, "/dev/%s", device_name);
 	if (ret < 0)
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH v2 2/3] tools: gpio: validate arguments in gpiotools_request_line

[Maxwell Doose]

On Mon, May 4, 2026 at 2:56 AM Zhang Xiaolei <zxl434815272@gmail.com> wrote:
>
> Add validation for input pointers and number of lines.
>

Perhaps make the commit message more descriptive?

>
> Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
> ---
>  tools/gpio/gpio-utils.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
> index 176bccfcccb0..930a38fe7911 100644
> --- a/tools/gpio/gpio-utils.c
> +++ b/tools/gpio/gpio-utils.c
> @@ -65,6 +65,12 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
>         int i;
>         int ret;
>
> +       if (!device_name || !lines || !config || !consumer || !num_lines)
> +               return -EINVAL;
> +
> +       if (num_lines > GPIO_V2_LINES_MAX)
> +               return -EINVAL;
> +

I'm wondering if we might want to use ERANGE here for the num_lines >
GPIO_V2_LINES_MAX check instead of EINVAL.

best regards,
maxwell

[PATCH v2 3/3] tools: gpio: fix ioctl name in error message

[Zhang Xiaolei]

Use the correct ioctl name in the error message.

Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
---
 tools/gpio/gpio-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
index 930a38fe7911..0d52d58cc6b6 100644
--- a/tools/gpio/gpio-utils.c
+++ b/tools/gpio/gpio-utils.c
@@ -95,7 +95,7 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
 	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));
 	}
 
 	if (close(fd) == -1)
-- 
2.34.1

Re: [PATCH v2 3/3] tools: gpio: fix ioctl name in error message

[Maxwell Doose]

Hi Zhang,

On Mon, May 4, 2026 at 2:56 AM Zhang Xiaolei <zxl434815272@gmail.com> wrote:
>
> Use the correct ioctl name in the error message.
>
> Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
> ---
>  tools/gpio/gpio-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
[snip]

Patch looks technically good but when I meant split these patches I
meant into entirely separate patches. Please split this away from the
patch series.

best regards,
max

Re: [PATCH v2 1/3] tools: gpio: use strscpy() for consumer name

[David Laight]

On Mon,  4 May 2026 15:50:34 +0800
Zhang Xiaolei <zxl434815272@gmail.com> wrote:

> Replace strcpy() with strscpy() to avoid potential buffer overflow
> when copying the consumer string.

You ought to run code before submitting patches.
This wasn't even compiled.

-- David

> 
> Signed-off-by: Zhang Xiaolei <zxl434815272@gmail.com>
> ---
>  tools/gpio/gpio-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/gpio/gpio-utils.c b/tools/gpio/gpio-utils.c
> index 4096bcd511d1..176bccfcccb0 100644
> --- a/tools/gpio/gpio-utils.c
> +++ b/tools/gpio/gpio-utils.c
> @@ -82,7 +82,7 @@ int gpiotools_request_line(const char *device_name, unsigned int *lines,
>  		req.offsets[i] = lines[i];
>  
>  	req.config = *config;
> -	strcpy(req.consumer, consumer);
> +	strcpy(req.consumer, consumer, sizeof(req.consumer));
>  	req.num_lines = num_lines;
>  
>  	ret = ioctl(fd, GPIO_V2_GET_LINE_IOCTL, &req);