[PATCH 0/8] gpiolib: sanitize return values of callbacks

[PATCH 0/8] gpiolib: sanitize return values of callbacks

[Bartosz Golaszewski]

We've had instances of drivers returning invalid values from gpio_chip
calbacks. In several cases these return values would be propagated to
user-space and confuse programs that only expect 0 or negative errnos
from ioctl()s. Let's sanitize the return values of callbacks and make
sure we don't allow anyone see invalid ones.

The first patch checks the return values of get_direction() in kernel
where needed and is a backportable fix.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (8):
      gpiolib: check the return value of gpio_chip::get_direction()
      gpiolib: sanitize the return value of gpio_chip::request()
      gpiolib: sanitize the return value of gpio_chip::set_config()
      gpiolib: sanitize the return value of gpio_chip::get()
      gpiolib: sanitize the return value of gpio_chip::get_multiple()
      gpiolib: sanitize the return value of gpio_chip::direction_output()
      gpiolib: sanitize the return value of gpio_chip::direction_input()
      gpiolib: sanitize the return value of gpio_chip::get_direction()

 drivers/gpio/gpiolib.c      | 144 +++++++++++++++++++++++++++++++++++---------
 include/linux/gpio/driver.h |   6 +-
 2 files changed, 120 insertions(+), 30 deletions(-)
---
base-commit: a13f6e0f405ed0d3bcfd37c692c7d7fa3c052154
change-id: 20241212-gpio-sanitize-retvals-f5f4e0d6f57d

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As per the API contract - gpio_chip::get_direction() may fail and return
a negative error number. However, we treat it as if it always returned 0
or 1. Check the return value of the callback and propagate the error
number up the stack.

Cc: stable@vger.kernel.org
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 679ed764cb14..5d3774dc748b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1057,8 +1057,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		desc->gdev = gdev;
 
 		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
-			assign_bit(FLAG_IS_OUT,
-				   &desc->flags, !gc->get_direction(gc, desc_index));
+			ret = gc->get_direction(gc, desc_index);
+			if (ret < 0)
+				goto err_cleanup_desc_srcu;
+
+			assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
 		} else {
 			assign_bit(FLAG_IS_OUT,
 				   &desc->flags, !gc->direction_input);
@@ -2728,13 +2731,18 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 	if (guard.gc->direction_input) {
 		ret = guard.gc->direction_input(guard.gc,
 						gpio_chip_hwgpio(desc));
-	} else if (guard.gc->get_direction &&
-		  (guard.gc->get_direction(guard.gc,
-					   gpio_chip_hwgpio(desc)) != 1)) {
-		gpiod_warn(desc,
-			   "%s: missing direction_input() operation and line is output\n",
-			   __func__);
-		return -EIO;
+	} else if (guard.gc->get_direction) {
+		ret = guard.gc->get_direction(guard.gc,
+					      gpio_chip_hwgpio(desc));
+		if (ret < 0)
+			return ret;
+
+		if (ret != GPIO_LINE_DIRECTION_IN) {
+			gpiod_warn(desc,
+				   "%s: missing direction_input() operation and line is output\n",
+				    __func__);
+			return -EIO;
+		}
 	}
 	if (ret == 0) {
 		clear_bit(FLAG_IS_OUT, &desc->flags);
@@ -2771,12 +2779,18 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 						 gpio_chip_hwgpio(desc), val);
 	} else {
 		/* Check that we are in output mode if we can */
-		if (guard.gc->get_direction &&
-		    guard.gc->get_direction(guard.gc, gpio_chip_hwgpio(desc))) {
-			gpiod_warn(desc,
-				"%s: missing direction_output() operation\n",
-				__func__);
-			return -EIO;
+		if (guard.gc->get_direction) {
+			ret = guard.gc->get_direction(guard.gc,
+						      gpio_chip_hwgpio(desc));
+			if (ret < 0)
+				return ret;
+
+			if (ret != GPIO_LINE_DIRECTION_OUT) {
+				gpiod_warn(desc,
+					   "%s: missing direction_output() operation\n",
+					   __func__);
+				return -EIO;
+			}
 		}
 		/*
 		 * If we can't actively set the direction, we are some

-- 
2.45.2

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3349 bytes --]

On Mon, Feb 10, 2025 at 11:51:55AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> As per the API contract - gpio_chip::get_direction() may fail and return
> a negative error number. However, we treat it as if it always returned 0
> or 1. Check the return value of the callback and propagate the error
> number up the stack.

This is breaking boot for me on both the original Raspberry Pi and the
Pi 4.  The boot dies without any output on the original Pi, on the Pi 4
the boot seems to die when disabling clocks:

[   11.695534] amba fe201000.serial: deferred probe pending: amba: wait for supplier /soc/gpio@7e200000/uart0-gpio32
[   11.705920] platform leds: deferred probe pending: leds-gpio: Failed to get GPIO '/leds/led-act'
[   15.032277] clk: Disabling unused clocks

Full log:

   https://lava.sirena.org.uk/scheduler/job/1126311

I've enclosed a bisect log below, it converges fairly smoothly:

git bisect start
# status: waiting for both good and bad commits
# bad: [67961d4f4e34f5ed1aeebab08f42c2e706837ec5] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect bad 67961d4f4e34f5ed1aeebab08f42c2e706837ec5
# status: waiting for good commit(s), bad commit known
# good: [6537cfb395f352782918d8ee7b7f10ba2cc3cbf2] Merge tag 'sound-6.14-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good 6537cfb395f352782918d8ee7b7f10ba2cc3cbf2
# good: [d59355014fa12fb0033edf64917ac0139cd6423a] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless.git
git bisect good d59355014fa12fb0033edf64917ac0139cd6423a
# good: [35c2c30101bf96517108fe969c4aad9e5c4f3614] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap.git
git bisect good 35c2c30101bf96517108fe969c4aad9e5c4f3614
# bad: [e52d7cc2f41223d070975c370f67686bd3213b41] Merge branch 'perf-tools' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
git bisect bad e52d7cc2f41223d070975c370f67686bd3213b41
# good: [163126388d62798769acd2cd1753839771dc12c6] Merge branch 'hyperv-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
git bisect good 163126388d62798769acd2cd1753839771dc12c6
# good: [5d176a6d15a456002e90e1776648396e7f0d57d3] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git
git bisect good 5d176a6d15a456002e90e1776648396e7f0d57d3
# bad: [c6d16b526a80a3215164f7e66c704dcb838e1810] Merge branch 'gpio/for-current' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect bad c6d16b526a80a3215164f7e66c704dcb838e1810
# bad: [81570d6a7ad37033c7895811551a5a9023706eda] gpiolib: protect gpio_chip with SRCU in array_info paths in multi get/set
git bisect bad 81570d6a7ad37033c7895811551a5a9023706eda
# bad: [4e667a1968099c6deadee2313ecd648f8f0a8956] gpio: vf610: add locking to gpio direction functions
git bisect bad 4e667a1968099c6deadee2313ecd648f8f0a8956
# bad: [9d846b1aebbe488f245f1aa463802ff9c34cc078] gpiolib: check the return value of gpio_chip::get_direction()
git bisect bad 9d846b1aebbe488f245f1aa463802ff9c34cc078
# first bad commit: [9d846b1aebbe488f245f1aa463802ff9c34cc078] gpiolib: check the return value of gpio_chip::get_direction()

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Marek Szyprowski]

Hi Bartosz,

On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As per the API contract - gpio_chip::get_direction() may fail and return
> a negative error number. However, we treat it as if it always returned 0
> or 1. Check the return value of the callback and propagate the error
> number up the stack.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++---------------
>   1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 679ed764cb14..5d3774dc748b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1057,8 +1057,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   		desc->gdev = gdev;
>   
>   		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> -			assign_bit(FLAG_IS_OUT,
> -				   &desc->flags, !gc->get_direction(gc, desc_index));
> +			ret = gc->get_direction(gc, desc_index);
> +			if (ret < 0)
> +				goto err_cleanup_desc_srcu;
> +
> +			assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
>   		} else {
>   			assign_bit(FLAG_IS_OUT,
>   				   &desc->flags, !gc->direction_input);

This change breaks bcm2835 pincontrol/gpio driver (and probably others) 
in next-20250218. The problem is that some gpio lines are initially 
configured as alternate function (i.e. uart) and .get_direction returns 
-EINVAL for them, what in turn causes the whole gpio chip fail to 
register. Here is the log with WARN_ON() added to line 
drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:

  ------------[ cut here ]------------
  WARNING: CPU: 0 PID: 1 at drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 
bcm2835_gpio_get_direction+0x80/0x98
  Modules linked in:
  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.14.0-rc3-next-20250218-dirty #9817
  Hardware name: Raspberry Pi 4 Model B (DT)
  pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : bcm2835_gpio_get_direction+0x80/0x98
  lr : bcm2835_gpio_get_direction+0x18/0x98
  ...
  Call trace:
   bcm2835_gpio_get_direction+0x80/0x98 (P)
   gpiochip_add_data_with_key+0x874/0xef0
   bcm2835_pinctrl_probe+0x354/0x53c
   platform_probe+0x68/0xdc
   really_probe+0xbc/0x298
   __driver_probe_device+0x78/0x12c
   driver_probe_device+0xdc/0x164
   __driver_attach+0x9c/0x1ac
   bus_for_each_dev+0x74/0xd4
   driver_attach+0x24/0x30
   bus_add_driver+0xe4/0x208
   driver_register+0x60/0x128
   __platform_driver_register+0x24/0x30
   bcm2835_pinctrl_driver_init+0x20/0x2c
   do_one_initcall+0x64/0x308
   kernel_init_freeable+0x280/0x4e8
   kernel_init+0x20/0x1d8
   ret_from_fork+0x10/0x20
  irq event stamp: 100380
  hardirqs last  enabled at (100379): [<ffff8000812d7d5c>] 
_raw_spin_unlock_irqrestore+0x74/0x78
  hardirqs last disabled at (100380): [<ffff8000812c8918>] el1_dbg+0x24/0x8c
  softirqs last  enabled at (93674): [<ffff80008005ed4c>] 
handle_softirqs+0x4c4/0x4dc
  softirqs last disabled at (93669): [<ffff8000800105a0>] 
__do_softirq+0x14/0x20
  ---[ end trace 0000000000000000 ]---
  gpiochip_add_data_with_key: GPIOs 512..569 (pinctrl-bcm2711) failed to 
register, -22
  pinctrl-bcm2835 fe200000.gpio: could not add GPIO chip
  pinctrl-bcm2835 fe200000.gpio: probe with driver pinctrl-bcm2835 
failed with error -22


Any suggestions how to fix this issue? Should we add 
GPIO_LINE_DIRECTION_UNKNOWN?


> @@ -2728,13 +2731,18 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>   	if (guard.gc->direction_input) {
>   		ret = guard.gc->direction_input(guard.gc,
>   						gpio_chip_hwgpio(desc));
> -	} else if (guard.gc->get_direction &&
> -		  (guard.gc->get_direction(guard.gc,
> -					   gpio_chip_hwgpio(desc)) != 1)) {
> -		gpiod_warn(desc,
> -			   "%s: missing direction_input() operation and line is output\n",
> -			   __func__);
> -		return -EIO;
> +	} else if (guard.gc->get_direction) {
> +		ret = guard.gc->get_direction(guard.gc,
> +					      gpio_chip_hwgpio(desc));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret != GPIO_LINE_DIRECTION_IN) {
> +			gpiod_warn(desc,
> +				   "%s: missing direction_input() operation and line is output\n",
> +				    __func__);
> +			return -EIO;
> +		}
>   	}
>   	if (ret == 0) {
>   		clear_bit(FLAG_IS_OUT, &desc->flags);
> @@ -2771,12 +2779,18 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>   						 gpio_chip_hwgpio(desc), val);
>   	} else {
>   		/* Check that we are in output mode if we can */
> -		if (guard.gc->get_direction &&
> -		    guard.gc->get_direction(guard.gc, gpio_chip_hwgpio(desc))) {
> -			gpiod_warn(desc,
> -				"%s: missing direction_output() operation\n",
> -				__func__);
> -			return -EIO;
> +		if (guard.gc->get_direction) {
> +			ret = guard.gc->get_direction(guard.gc,
> +						      gpio_chip_hwgpio(desc));
> +			if (ret < 0)
> +				return ret;
> +
> +			if (ret != GPIO_LINE_DIRECTION_OUT) {
> +				gpiod_warn(desc,
> +					   "%s: missing direction_output() operation\n",
> +					   __func__);
> +				return -EIO;
> +			}
>   		}
>   		/*
>   		 * If we can't actively set the direction, we are some
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

On Wed, Feb 19, 2025 at 9:38 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Bartosz,
>
> On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract - gpio_chip::get_direction() may fail and return
> > a negative error number. However, we treat it as if it always returned 0
> > or 1. Check the return value of the callback and propagate the error
> > number up the stack.
> >
>
> This change breaks bcm2835 pincontrol/gpio driver (and probably others)
> in next-20250218. The problem is that some gpio lines are initially
> configured as alternate function (i.e. uart) and .get_direction returns
> -EINVAL for them, what in turn causes the whole gpio chip fail to
> register. Here is the log with WARN_ON() added to line
> drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:
>
> Any suggestions how to fix this issue? Should we add
> GPIO_LINE_DIRECTION_UNKNOWN?
>

That would be quite an intrusive change and not something for the
middle of the release cycle. I think we need to revert to the previous
behavior for this particular use-case: check ret for EINVAL and assume
it means input as it's the "safe" setting. Now the question is - can
this only happen during the chip registration or should we filter out
EINVAL at each gpiod_get_direction() call?

Bart

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Marek Szyprowski]

On 19.02.2025 09:50, Bartosz Golaszewski wrote:
> On Wed, Feb 19, 2025 at 9:38 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 10.02.2025 11:51, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> As per the API contract - gpio_chip::get_direction() may fail and return
>>> a negative error number. However, we treat it as if it always returned 0
>>> or 1. Check the return value of the callback and propagate the error
>>> number up the stack.
>>>
>> This change breaks bcm2835 pincontrol/gpio driver (and probably others)
>> in next-20250218. The problem is that some gpio lines are initially
>> configured as alternate function (i.e. uart) and .get_direction returns
>> -EINVAL for them, what in turn causes the whole gpio chip fail to
>> register. Here is the log with WARN_ON() added to line
>> drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:
>>
>> Any suggestions how to fix this issue? Should we add
>> GPIO_LINE_DIRECTION_UNKNOWN?
>>
> That would be quite an intrusive change and not something for the
> middle of the release cycle. I think we need to revert to the previous
> behavior for this particular use-case: check ret for EINVAL and assume
> it means input as it's the "safe" setting. Now the question is - can
> this only happen during the chip registration or should we filter out
> EINVAL at each gpiod_get_direction() call?

IMHO it will be enough to use that workaround only in the 
gpiochip_add_data_with_key() function. The other functions modified by 
the $subject patch are strictly related to input or output gpio mode of 
operation, so having the line set to proper input/output state seems to 
be justified.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

On Wed, Feb 19, 2025 at 10:14 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 19.02.2025 09:50, Bartosz Golaszewski wrote:
> > On Wed, Feb 19, 2025 at 9:38 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>> As per the API contract - gpio_chip::get_direction() may fail and return
> >>> a negative error number. However, we treat it as if it always returned 0
> >>> or 1. Check the return value of the callback and propagate the error
> >>> number up the stack.
> >>>
> >> This change breaks bcm2835 pincontrol/gpio driver (and probably others)
> >> in next-20250218. The problem is that some gpio lines are initially
> >> configured as alternate function (i.e. uart) and .get_direction returns
> >> -EINVAL for them, what in turn causes the whole gpio chip fail to
> >> register. Here is the log with WARN_ON() added to line
> >> drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:
> >>
> >> Any suggestions how to fix this issue? Should we add
> >> GPIO_LINE_DIRECTION_UNKNOWN?
> >>
> > That would be quite an intrusive change and not something for the
> > middle of the release cycle. I think we need to revert to the previous
> > behavior for this particular use-case: check ret for EINVAL and assume
> > it means input as it's the "safe" setting. Now the question is - can
> > this only happen during the chip registration or should we filter out
> > EINVAL at each gpiod_get_direction() call?
>
> IMHO it will be enough to use that workaround only in the
> gpiochip_add_data_with_key() function. The other functions modified by
> the $subject patch are strictly related to input or output gpio mode of
> operation, so having the line set to proper input/output state seems to
> be justified.
>

Cc'ing Florian

After a quick glance at existing get_direction() callbacks, it seems
this is the only driver that does it. I'm wondering if it wouldn't
make sense to change the driver behavior instead and make it assume
input for unknown functions.

Bart

Re: [PATCH 1/8] gpiolib: check the return value of gpio_chip::get_direction()

[Antonio Borneo]

On Wed, 2025-02-19 at 09:38 +0100, Marek Szyprowski wrote:
> Hi Bartosz,
> 
> On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > As per the API contract - gpio_chip::get_direction() may fail and return
> > a negative error number. However, we treat it as if it always returned 0
> > or 1. Check the return value of the callback and propagate the error
> > number up the stack.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++---------------
> >   1 file changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 679ed764cb14..5d3774dc748b 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1057,8 +1057,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >                 desc->gdev = gdev;
> >   
> >                 if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> > -                       assign_bit(FLAG_IS_OUT,
> > -                                  &desc->flags, !gc->get_direction(gc, desc_index));
> > +                       ret = gc->get_direction(gc, desc_index);
> > +                       if (ret < 0)
> > +                               goto err_cleanup_desc_srcu;
> > +
> > +                       assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
> >                 } else {
> >                         assign_bit(FLAG_IS_OUT,
> >                                    &desc->flags, !gc->direction_input);
> 
> This change breaks bcm2835 pincontrol/gpio driver (and probably others) 
> in next-20250218. The problem is that some gpio lines are initially 
> configured as alternate function (i.e. uart) and .get_direction returns 
> -EINVAL for them, what in turn causes the whole gpio chip fail to 
> register. Here is the log with WARN_ON() added to line 
> drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:

Same issue with STM32 pinctrl.

I will send out shortly a patch, similar to
https://lore.kernel.org/all/20250219102750.38519-1-brgl@bgdev.pl/

Regards,
Antonio

[PATCH 2/8] gpiolib: sanitize the return value of gpio_chip::request()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The return value of the request() callback may be propagated to
user-space. If a bad driver returns a positive number, it may confuse
user programs. Tighten the API contract and check for positive numbers
returned by GPIO controllers.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 2 ++
 include/linux/gpio/driver.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5d3774dc748b..42625da7e797 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2331,6 +2331,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 			ret = guard.gc->request(guard.gc, offset);
 		else
 			ret = -EINVAL;
+		if (ret > 0)
+			ret = -EBADE;
 		if (ret)
 			goto out_clear_bit;
 	}
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270..26fcddcb74b8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -328,7 +328,8 @@ struct gpio_irq_chip {
  * @fwnode: optional fwnode providing this controller's properties
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @request: optional hook for chip-specific activation, such as
- *	enabling module power and clock; may sleep
+ *	enabling module power and clock; may sleep; must return 0 on success
+ *	or negative error number on failure
  * @free: optional hook for chip-specific deactivation, such as
  *	disabling module power and clock; may sleep
  * @get_direction: returns direction for signal "offset", 0=out, 1=in,

-- 
2.45.2

[PATCH 3/8] gpiolib: sanitize the return value of gpio_chip::set_config()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The return value of the set_config() callback may be propagated to
user-space. If a bad driver returns a positive number, it may confuse
user programs. Tighten the API contract and check for positive numbers
returned by GPIO controllers.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c      | 3 +++
 include/linux/gpio/driver.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 42625da7e797..95ea300da109 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2575,6 +2575,9 @@ int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
 		return -ENOTSUPP;
 
 	ret = guard.gc->set_config(guard.gc, gpio_chip_hwgpio(desc), config);
+	if (ret > 0)
+		ret = -EBADE;
+
 #ifdef CONFIG_GPIO_CDEV
 	/*
 	 * Special case - if we're setting debounce period, we need to store
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 26fcddcb74b8..81c06fc8dcda 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -348,7 +348,8 @@ struct gpio_irq_chip {
  * @set: assigns output value for signal "offset"
  * @set_multiple: assigns output values for multiple signals defined by "mask"
  * @set_config: optional hook for all kinds of settings. Uses the same
- *	packed config format as generic pinconf.
+ *	packed config format as generic pinconf. Must return 0 on success and
+ *	a negative error number on failure.
  * @to_irq: optional hook supporting non-static gpiod_to_irq() mappings;
  *	implementation may not sleep
  * @dbg_show: optional routine to show contents in debugfs; default code

-- 
2.45.2

[PATCH 4/8] gpiolib: sanitize the return value of gpio_chip::get()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As per the API contract, the get() callback is only allowed to return 0,
1 or a negative error number. Add a wrapper around the callback calls
that filters out anything else.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 95ea300da109..7bc316154fdc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3097,9 +3097,25 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
 
+static int gpiochip_get(struct gpio_chip *gc, unsigned int offset)
+{
+	int ret;
+
+	lockdep_assert_held(&gc->gpiodev->srcu);
+
+	if (!gc->get)
+		return -EIO;
+
+	ret = gc->get(gc, offset);
+	if (ret > 1)
+		ret = -EBADE;
+
+	return ret;
+}
+
 static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
 {
-	return gc->get ? gc->get(gc, gpio_chip_hwgpio(desc)) : -EIO;
+	return gpiochip_get(gc, gpio_chip_hwgpio(desc));
 }
 
 /* I/O calls are only valid after configuration completed; the relevant
@@ -3154,7 +3170,7 @@ static int gpio_chip_get_multiple(struct gpio_chip *gc,
 		int i, value;
 
 		for_each_set_bit(i, mask, gc->ngpio) {
-			value = gc->get(gc, i);
+			value = gpiochip_get(gc, i);
 			if (value < 0)
 				return value;
 			__assign_bit(i, bits, value);

-- 
2.45.2

Re: [PATCH 4/8] gpiolib: sanitize the return value of gpio_chip::get()

[Andy Shevchenko]

On Mon, Feb 10, 2025 at 11:51:58AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> As per the API contract, the get() callback is only allowed to return 0,
> 1 or a negative error number. Add a wrapper around the callback calls
> that filters out anything else.

...

> +static int gpiochip_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&gc->gpiodev->srcu);
> +
> +	if (!gc->get)
> +		return -EIO;
> +
> +	ret = gc->get(gc, offset);
> +	if (ret > 1)

Perhaps use the respective GPIO macro instead? Otherwise it's not clear what
the meaning of 1 is.

> +		ret = -EBADE;
> +
> +	return ret;
> +}
> +
>  static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
>  {
> -	return gc->get ? gc->get(gc, gpio_chip_hwgpio(desc)) : -EIO;
> +	return gpiochip_get(gc, gpio_chip_hwgpio(desc));
>  }

...

>  		for_each_set_bit(i, mask, gc->ngpio) {
> -			value = gc->get(gc, i);
> +			value = gpiochip_get(gc, i);

This will delay the function for checking every time if the get() exists. Which
must be here.

>  			if (value < 0)
>  				return value;
>  			__assign_bit(i, bits, value);

What I would expect here is something like this:

static int gpio_chip_get_value_nocheck(struct gpio_chip *gc, unsigned int offset)
{
	int ret;

	lockdep_assert_held(&gc->gpiodev->srcu);

	ret = gc->get(gc, offset);
	if (ret > GPIO_LINE_DIRECTION_IN)
		ret = -EBADE;

	return ret;
}

static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
{
	return gc->get ? gpio_chip_get_value_nocheck(gc, gpio_chip_hwgpio(desc)) : -EIO;
}

But I see the downside of it as it might lurk without RCU lock if get is not
defined. So, up to you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] gpiolib: sanitize the return value of gpio_chip::get()

[Bartosz Golaszewski]

On Mon, Feb 24, 2025 at 5:30 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Feb 10, 2025 at 11:51:58AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract, the get() callback is only allowed to return 0,
> > 1 or a negative error number. Add a wrapper around the callback calls
> > that filters out anything else.
>
> ...
>
> > +static int gpiochip_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     int ret;
> > +
> > +     lockdep_assert_held(&gc->gpiodev->srcu);
> > +
> > +     if (!gc->get)
> > +             return -EIO;
> > +
> > +     ret = gc->get(gc, offset);
> > +     if (ret > 1)
>
> Perhaps use the respective GPIO macro instead? Otherwise it's not clear what
> the meaning of 1 is.
>

We don't have one for GPIO values.


> > +             ret = -EBADE;
> > +
> > +     return ret;
> > +}
> > +
> >  static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
> >  {
> > -     return gc->get ? gc->get(gc, gpio_chip_hwgpio(desc)) : -EIO;
> > +     return gpiochip_get(gc, gpio_chip_hwgpio(desc));
> >  }
>
> ...
>
> >               for_each_set_bit(i, mask, gc->ngpio) {
> > -                     value = gc->get(gc, i);
> > +                     value = gpiochip_get(gc, i);
>
> This will delay the function for checking every time if the get() exists. Which
> must be here.
>
> >                       if (value < 0)
> >                               return value;
> >                       __assign_bit(i, bits, value);
>
> What I would expect here is something like this:
>
> static int gpio_chip_get_value_nocheck(struct gpio_chip *gc, unsigned int offset)
> {
>         int ret;
>
>         lockdep_assert_held(&gc->gpiodev->srcu);
>
>         ret = gc->get(gc, offset);
>         if (ret > GPIO_LINE_DIRECTION_IN)
>                 ret = -EBADE;
>
>         return ret;
> }
>
> static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
> {
>         return gc->get ? gpio_chip_get_value_nocheck(gc, gpio_chip_hwgpio(desc)) : -EIO;
> }
>
> But I see the downside of it as it might lurk without RCU lock if get is not
> defined. So, up to you.
>

Makes sense, gpiochip_get() is only called in gpio_chip_get_value()
and gpiochip_get_multiple() where gc->get is already checked.

Bart

[PATCH 5/8] gpiolib: sanitize the return value of gpio_chip::get_multiple()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As per the API contract, the get_multiple() callback is only allowed to
return 0 or a negative error number. Filter out anything else.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7bc316154fdc..7255f4df6a8b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3164,8 +3164,13 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
 static int gpio_chip_get_multiple(struct gpio_chip *gc,
 				  unsigned long *mask, unsigned long *bits)
 {
-	if (gc->get_multiple)
-		return gc->get_multiple(gc, mask, bits);
+	int ret;
+
+	if (gc->get_multiple) {
+		ret = gc->get_multiple(gc, mask, bits);
+		if (ret > 0)
+			return -EBADE;
+	}
 	if (gc->get) {
 		int i, value;
 

-- 
2.45.2

[PATCH 6/8] gpiolib: sanitize the return value of gpio_chip::direction_output()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The return value of the direction_output() callback may be propagated to
user-space. As per the API contract it can only return 0 or a negative
error number. Add a wrapper around the callback calls that filters out
anything else.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7255f4df6a8b..1f75ae6e208c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2683,6 +2683,23 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 	return ret;
 }
 
+static int gpiochip_direction_output(struct gpio_chip *gc, unsigned int offset,
+				     int value)
+{
+	int ret;
+
+	lockdep_assert_held(&gc->gpiodev->srcu);
+
+	if (WARN_ON(!gc->direction_output))
+		return -EOPNOTSUPP;
+
+	ret = gc->direction_output(gc, offset, value);
+	if (ret > 0)
+		ret = -EBADE;
+
+	return ret;
+}
+
 /**
  * gpiod_direction_input - set the GPIO direction to input
  * @desc:	GPIO to set to input
@@ -2780,8 +2797,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 	}
 
 	if (guard.gc->direction_output) {
-		ret = guard.gc->direction_output(guard.gc,
-						 gpio_chip_hwgpio(desc), val);
+		ret = gpiochip_direction_output(guard.gc,
+						gpio_chip_hwgpio(desc), val);
 	} else {
 		/* Check that we are in output mode if we can */
 		if (guard.gc->get_direction) {
@@ -3433,7 +3450,7 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 	if (value) {
 		ret = guard.gc->direction_input(guard.gc, offset);
 	} else {
-		ret = guard.gc->direction_output(guard.gc, offset, 0);
+		ret = gpiochip_direction_output(guard.gc, offset, 0);
 		if (!ret)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	}
@@ -3458,7 +3475,7 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 		return;
 
 	if (value) {
-		ret = guard.gc->direction_output(guard.gc, offset, 1);
+		ret = gpiochip_direction_output(guard.gc, offset, 1);
 		if (!ret)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	} else {

-- 
2.45.2

[PATCH 7/8] gpiolib: sanitize the return value of gpio_chip::direction_input()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The return value of the direction_input() callback may be propagated to
user-space. As per the API contract it can only return 0 or a negative
error number. Add a wrapper around the callback calls that filters out
anything else.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1f75ae6e208c..683a03d237c0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2683,6 +2683,22 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 	return ret;
 }
 
+static int gpiochip_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	int ret;
+
+	lockdep_assert_held(&gc->gpiodev->srcu);
+
+	if (WARN_ON(!gc->direction_input))
+		return -EOPNOTSUPP;
+
+	ret = gc->direction_input(gc, offset);
+	if (ret > 0)
+		ret = -EBADE;
+
+	return ret;
+}
+
 static int gpiochip_direction_output(struct gpio_chip *gc, unsigned int offset,
 				     int value)
 {
@@ -2751,8 +2767,8 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 	 * assume we are in input mode after this.
 	 */
 	if (guard.gc->direction_input) {
-		ret = guard.gc->direction_input(guard.gc,
-						gpio_chip_hwgpio(desc));
+		ret = gpiochip_direction_input(guard.gc,
+					       gpio_chip_hwgpio(desc));
 	} else if (guard.gc->get_direction) {
 		ret = guard.gc->get_direction(guard.gc,
 					      gpio_chip_hwgpio(desc));
@@ -3448,7 +3464,7 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 		return;
 
 	if (value) {
-		ret = guard.gc->direction_input(guard.gc, offset);
+		ret = gpiochip_direction_input(guard.gc, offset);
 	} else {
 		ret = gpiochip_direction_output(guard.gc, offset, 0);
 		if (!ret)
@@ -3479,7 +3495,7 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 		if (!ret)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		ret = guard.gc->direction_input(guard.gc, offset);
+		ret = gpiochip_direction_input(guard.gc, offset);
 	}
 	trace_gpio_direction(desc_to_gpio(desc), !value, ret);
 	if (ret < 0)

-- 
2.45.2

[PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As per the API contract, the get_direction() callback can only
return 0, 1 or a negative error number. Add a wrapper around the callback
calls that filters out anything else.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 683a03d237c0..7f2aca9f81a1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
 	}
 }
 
+static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	int ret;
+
+	lockdep_assert_held(&gc->gpiodev->srcu);
+
+	if (WARN_ON(!gc->get_direction))
+		return -EOPNOTSUPP;
+
+	ret = gc->get_direction(gc, offset);
+	if (ret > 1)
+		ret = -EBADE;
+
+	return ret;
+}
+
 /**
  * gpiod_get_direction - return the current direction of a GPIO
  * @desc:	GPIO to get the direction of
@@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	if (!guard.gc->get_direction)
 		return -ENOTSUPP;
 
-	ret = guard.gc->get_direction(guard.gc, offset);
+	ret = gpiochip_get_direction(guard.gc, offset);
 	if (ret < 0)
 		return ret;
 
@@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		desc->gdev = gdev;
 
 		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
-			ret = gc->get_direction(gc, desc_index);
+			ret = gpiochip_get_direction(gc, desc_index);
 			if (ret < 0)
 				goto err_cleanup_desc_srcu;
 
@@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
 		ret = gpiochip_direction_input(guard.gc,
 					       gpio_chip_hwgpio(desc));
 	} else if (guard.gc->get_direction) {
-		ret = guard.gc->get_direction(guard.gc,
-					      gpio_chip_hwgpio(desc));
+		ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc));
 		if (ret < 0)
 			return ret;
 
@@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 	} else {
 		/* Check that we are in output mode if we can */
 		if (guard.gc->get_direction) {
-			ret = guard.gc->get_direction(guard.gc,
-						      gpio_chip_hwgpio(desc));
+			ret = gpiochip_get_direction(guard.gc,
+						     gpio_chip_hwgpio(desc));
 			if (ret < 0)
 				return ret;
 

-- 
2.45.2

Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Andy Shevchenko]

On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> As per the API contract, the get_direction() callback can only
> return 0, 1 or a negative error number. Add a wrapper around the callback
> calls that filters out anything else.

...

> +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&gc->gpiodev->srcu);
> +
> +	if (WARN_ON(!gc->get_direction))
> +		return -EOPNOTSUPP;
> +
> +	ret = gc->get_direction(gc, offset);
> +	if (ret > 1)

Would it be better to use the respective GPIO*... macro instead of 1?

> +		ret = -EBADE;
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract, the get_direction() callback can only
> > return 0, 1 or a negative error number. Add a wrapper around the callback
> > calls that filters out anything else.
>
> ...
>
> > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     int ret;
> > +
> > +     lockdep_assert_held(&gc->gpiodev->srcu);
> > +
> > +     if (WARN_ON(!gc->get_direction))
> > +             return -EOPNOTSUPP;
> > +
> > +     ret = gc->get_direction(gc, offset);
> > +     if (ret > 1)
>
> Would it be better to use the respective GPIO*... macro instead of 1?
>

I did consider it but I don't like comparing against enums, it doesn't
feel right as the value behind the name can change. I think I prefer
it like this even if it's not the best solution either. Maybe we could
be more explicit and say:

    if (!(ret == IN || ret == OUT || ret < 0)

?

Bart

Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Andy Shevchenko]

Mon, Feb 24, 2025 at 08:55:26PM +0100, Bartosz Golaszewski kirjoitti:
> On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:

...

> > > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > +     int ret;
> > > +
> > > +     lockdep_assert_held(&gc->gpiodev->srcu);
> > > +
> > > +     if (WARN_ON(!gc->get_direction))
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     ret = gc->get_direction(gc, offset);
> > > +     if (ret > 1)
> >
> > Would it be better to use the respective GPIO*... macro instead of 1?
> >
> 
> I did consider it but I don't like comparing against enums, it doesn't
> feel right as the value behind the name can change. I think I prefer
> it like this even if it's not the best solution either. Maybe we could
> be more explicit and say:
> 
>     if (!(ret == IN || ret == OUT || ret < 0)
> 
> ?

Yep, I like this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Marek Szyprowski]

On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As per the API contract, the get_direction() callback can only
> return 0, 1 or a negative error number. Add a wrapper around the callback
> calls that filters out anything else.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This patch landed in today's linux-next as commit e623c4303ed11 
("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It 
introduced a lockdep warning from the gpiochip_get_direction() function. 
IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu 
locking/annotation for the newly created gpio chip. Here is the log:

gpio gpiochip1: Static allocation of GPIO base is deprecated, use 
dynamic allocation.
------------[ cut here ]------------
WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349 
gpiochip_get_direction+0x48/0x66
Modules linked in: cdns_usb_common roles cdns3_starfive 
snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx 
clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp 
jh7110_trng sfctemp dwmac_starfive stmmac_platform 
spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac 
clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022 
phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm 
drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables
CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W          
6.14.0-rc4-next-20250225 #1054
Tainted: [W]=WARN
Hardware name: StarFive VisionFive 2 v1.2A (DT)
Workqueue: events_unbound deferred_probe_work_func
epc : gpiochip_get_direction+0x48/0x66
  ra : gpiochip_get_direction+0x46/0x66
...
[<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66
[<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2
[<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a
[<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa
[<ffffffff80731116>] platform_probe+0x4e/0x92
[<ffffffff8000c366>] really_probe+0x10a/0x2de
[<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0
[<ffffffff8072ee34>] driver_probe_device+0x78/0xc4
[<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6
[<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0
[<ffffffff8072f33e>] __device_attach+0x84/0x13c
[<ffffffff8072f55e>] device_initial_probe+0xe/0x16
[<ffffffff8072e002>] bus_probe_device+0x88/0x8a
[<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee
[<ffffffff80047b7e>] process_one_work+0x1d0/0x57a
[<ffffffff8004854e>] worker_thread+0x166/0x2cc
[<ffffffff80051568>] kthread+0xdc/0x1b4
[<ffffffff80bcb942>] ret_from_fork+0xe/0x18
irq event stamp: 17857
hardirqs last  enabled at (17857): [<ffffffff80bca986>] 
_raw_spin_unlock_irqrestore+0x4c/0x4e
hardirqs last disabled at (17856): [<ffffffff80bca73c>] 
_raw_spin_lock_irqsave+0x5e/0x64
softirqs last  enabled at (17322): [<ffffffff80adff1a>] 
inet6_fill_ifla6_attrs+0x3d0/0x420
softirqs last disabled at (17320): [<ffffffff80adfefe>] 
inet6_fill_ifla6_attrs+0x3b4/0x420
---[ end trace 0000000000000000 ]---


> ---
>   drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 683a03d237c0..7f2aca9f81a1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
>   	}
>   }
>   
> +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&gc->gpiodev->srcu);
> +
> +	if (WARN_ON(!gc->get_direction))
> +		return -EOPNOTSUPP;
> +
> +	ret = gc->get_direction(gc, offset);
> +	if (ret > 1)
> +		ret = -EBADE;
> +
> +	return ret;
> +}
> +
>   /**
>    * gpiod_get_direction - return the current direction of a GPIO
>    * @desc:	GPIO to get the direction of
> @@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
>   	if (!guard.gc->get_direction)
>   		return -ENOTSUPP;
>   
> -	ret = guard.gc->get_direction(guard.gc, offset);
> +	ret = gpiochip_get_direction(guard.gc, offset);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   		desc->gdev = gdev;
>   
>   		if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> -			ret = gc->get_direction(gc, desc_index);
> +			ret = gpiochip_get_direction(gc, desc_index);
>   			if (ret < 0)
>   				goto err_cleanup_desc_srcu;
>   
> @@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>   		ret = gpiochip_direction_input(guard.gc,
>   					       gpio_chip_hwgpio(desc));
>   	} else if (guard.gc->get_direction) {
> -		ret = guard.gc->get_direction(guard.gc,
> -					      gpio_chip_hwgpio(desc));
> +		ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc));
>   		if (ret < 0)
>   			return ret;
>   
> @@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>   	} else {
>   		/* Check that we are in output mode if we can */
>   		if (guard.gc->get_direction) {
> -			ret = guard.gc->get_direction(guard.gc,
> -						      gpio_chip_hwgpio(desc));
> +			ret = gpiochip_get_direction(guard.gc,
> +						     gpio_chip_hwgpio(desc));
>   			if (ret < 0)
>   				return ret;
>   
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Bartosz Golaszewski]

On Tue, Feb 25, 2025 at 11:13 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract, the get_direction() callback can only
> > return 0, 1 or a negative error number. Add a wrapper around the callback
> > calls that filters out anything else.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This patch landed in today's linux-next as commit e623c4303ed11
> ("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It
> introduced a lockdep warning from the gpiochip_get_direction() function.
> IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu
> locking/annotation for the newly created gpio chip. Here is the log:
>
> gpio gpiochip1: Static allocation of GPIO base is deprecated, use
> dynamic allocation.
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349
> gpiochip_get_direction+0x48/0x66
> Modules linked in: cdns_usb_common roles cdns3_starfive
> snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx
> clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp
> jh7110_trng sfctemp dwmac_starfive stmmac_platform
> spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac
> clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022
> phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm
> drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables
> CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W
> 6.14.0-rc4-next-20250225 #1054
> Tainted: [W]=WARN
> Hardware name: StarFive VisionFive 2 v1.2A (DT)
> Workqueue: events_unbound deferred_probe_work_func
> epc : gpiochip_get_direction+0x48/0x66
>   ra : gpiochip_get_direction+0x46/0x66
> ...
> [<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66
> [<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2
> [<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a
> [<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa
> [<ffffffff80731116>] platform_probe+0x4e/0x92
> [<ffffffff8000c366>] really_probe+0x10a/0x2de
> [<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0
> [<ffffffff8072ee34>] driver_probe_device+0x78/0xc4
> [<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6
> [<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0
> [<ffffffff8072f33e>] __device_attach+0x84/0x13c
> [<ffffffff8072f55e>] device_initial_probe+0xe/0x16
> [<ffffffff8072e002>] bus_probe_device+0x88/0x8a
> [<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee
> [<ffffffff80047b7e>] process_one_work+0x1d0/0x57a
> [<ffffffff8004854e>] worker_thread+0x166/0x2cc
> [<ffffffff80051568>] kthread+0xdc/0x1b4
> [<ffffffff80bcb942>] ret_from_fork+0xe/0x18
> irq event stamp: 17857
> hardirqs last  enabled at (17857): [<ffffffff80bca986>]
> _raw_spin_unlock_irqrestore+0x4c/0x4e
> hardirqs last disabled at (17856): [<ffffffff80bca73c>]
> _raw_spin_lock_irqsave+0x5e/0x64
> softirqs last  enabled at (17322): [<ffffffff80adff1a>]
> inet6_fill_ifla6_attrs+0x3d0/0x420
> softirqs last disabled at (17320): [<ffffffff80adfefe>]
> inet6_fill_ifla6_attrs+0x3b4/0x420
> ---[ end trace 0000000000000000 ]---
>

Thanks for the report. We don't need to hold the SRCU when registering
the chip. I'm now thinking I should revert using
gpiochip_get_direction() in gpiochip_add_data_with_key() to directly
calling the get_direction() callback and not checking its return
value.

Bartosz

Re: [PATCH 0/8] gpiolib: sanitize return values of callbacks

[Linus Walleij]

On Mon, Feb 10, 2025 at 11:52 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> We've had instances of drivers returning invalid values from gpio_chip
> calbacks. In several cases these return values would be propagated to
> user-space and confuse programs that only expect 0 or negative errnos
> from ioctl()s. Let's sanitize the return values of callbacks and make
> sure we don't allow anyone see invalid ones.
>
> The first patch checks the return values of get_direction() in kernel
> where needed and is a backportable fix.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This seems reasonable.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: (subset) [PATCH 0/8] gpiolib: sanitize return values of callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Mon, 10 Feb 2025 11:51:54 +0100, Bartosz Golaszewski wrote:
> We've had instances of drivers returning invalid values from gpio_chip
> calbacks. In several cases these return values would be propagated to
> user-space and confuse programs that only expect 0 or negative errnos
> from ioctl()s. Let's sanitize the return values of callbacks and make
> sure we don't allow anyone see invalid ones.
> 
> The first patch checks the return values of get_direction() in kernel
> where needed and is a backportable fix.
> 
> [...]

Queued this one for fixes. The rest will be picked up next week once this
is upstream.

[1/8] gpiolib: check the return value of gpio_chip::get_direction()
      commit: 9d846b1aebbe488f245f1aa463802ff9c34cc078

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 0/8] gpiolib: sanitize return values of callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Mon, 10 Feb 2025 11:51:54 +0100, Bartosz Golaszewski wrote:
> We've had instances of drivers returning invalid values from gpio_chip
> calbacks. In several cases these return values would be propagated to
> user-space and confuse programs that only expect 0 or negative errnos
> from ioctl()s. Let's sanitize the return values of callbacks and make
> sure we don't allow anyone see invalid ones.
> 
> The first patch checks the return values of get_direction() in kernel
> where needed and is a backportable fix.
> 
> [...]

Applied, thanks!

[1/8] gpiolib: check the return value of gpio_chip::get_direction()
      commit: 9d846b1aebbe488f245f1aa463802ff9c34cc078
[2/8] gpiolib: sanitize the return value of gpio_chip::request()
      commit: 69920338f8130da929ade6f93e6fa3e0e68433ee
[3/8] gpiolib: sanitize the return value of gpio_chip::set_config()
      commit: dcf8f3bffa2de2c7f3b5771b63605194ccd2286f
[4/8] gpiolib: sanitize the return value of gpio_chip::get()
      commit: 86ef402d805d606a10e6da8e5a64a51f6f5fb7e2
[5/8] gpiolib: sanitize the return value of gpio_chip::get_multiple()
      commit: 74abd086d2ee5ef10f68848cfe39e271ac798685
[6/8] gpiolib: sanitize the return value of gpio_chip::direction_output()
      commit: dfeb70c86d637d49af9313245e6b1f2ead08ce9b
[7/8] gpiolib: sanitize the return value of gpio_chip::direction_input()
      commit: 4750ddce95ae8be618e31b0aa51efcf50e23a78e
[8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()
      commit: e623c4303ed112a1fc20aec8427ba8407e2842e6

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>