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,
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
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()
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);
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;}@@ -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
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
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
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
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
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,
linux-stable-mirror@lists.linaro.org
-
Bartosz Golaszewski -
Linus Walleij -
Marek Szyprowski -
Mark Brown