[PATCH] gpiolib: of: fix bounds check for valid mask

[PATCH] gpiolib: of: fix bounds check for valid mask

[Andrei Lalaev]

Use "greater" instead of "greater or equal" when performs bounds check
to make sure that GPIOS are in available range. Previous implementation
skipped ranges which include last GPIO in range.

Signed-off-by: Andrei Lalaev <andrei.lalaev@emlid.com>
---
 drivers/gpio/gpiolib-of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index ae1ce319cd78..7e5e51d49d09 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -910,7 +910,7 @@ static void of_gpiochip_init_valid_mask(struct gpio_chip *chip)
 					   i, &start);
 		of_property_read_u32_index(np, "gpio-reserved-ranges",
 					   i + 1, &count);
-		if (start >= chip->ngpio || start + count >= chip->ngpio)
+		if (start >= chip->ngpio || start + count > chip->ngpio)
 			continue;
 
 		bitmap_clear(chip->valid_mask, start, count);
-- 
2.25.1

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Bartosz Golaszewski]

On Mon, Apr 11, 2022 at 8:36 AM Andrei Lalaev <andrei.lalaev@emlid.com> wrote:
>
> Use "greater" instead of "greater or equal" when performs bounds check
> to make sure that GPIOS are in available range. Previous implementation
> skipped ranges which include last GPIO in range.
>
> Signed-off-by: Andrei Lalaev <andrei.lalaev@emlid.com>
> ---
>  drivers/gpio/gpiolib-of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index ae1ce319cd78..7e5e51d49d09 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -910,7 +910,7 @@ static void of_gpiochip_init_valid_mask(struct gpio_chip *chip)
>                                            i, &start);
>                 of_property_read_u32_index(np, "gpio-reserved-ranges",
>                                            i + 1, &count);
> -               if (start >= chip->ngpio || start + count >= chip->ngpio)
> +               if (start >= chip->ngpio || start + count > chip->ngpio)
>                         continue;
>
>                 bitmap_clear(chip->valid_mask, start, count);
> --
> 2.25.1
>

Queued for fixes, good catch.

Thanks
Bart

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andy Shevchenko]

On Mon, Apr 11, 2022 at 12:57 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote:
>
> Use "greater" instead of "greater or equal" when performs bounds check
> to make sure that GPIOS are in available range. Previous implementation

the available

> skipped ranges which include last GPIO in range.

the last

Should it have a Fixes tag?

OTOH, the current implementation suggests that we have start,end
rather than start,count. What does documentation tell about it? Does
it need to be fixed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andrei Lalaev]

Thanks for the grammar comments.

> Should it have a Fixes tag?

Sure, thanks, I will resend with a Fixes tag and without grammar errors.

> What does documentation tell about it?

Documentation (devicetree/bindings/gpio/gpio.txt line 152) tells that
"This property indicates the start and size of the GPIOs that can't be used."
And the example (line 178) at the same file shows that the second element of
a tuple is the count: "gpio-reserved-ranges = <0 4>, <12 2>;"

> Does it need to be fixed?

I think so, because the current implementation doesn't reserve some GPIO ranges.
For example, we have 20 GPIOs and we want to reserve GPIOs from 14 to 19.
In this case the "reserved-ranges" looks like "<14 6>" but the
"of_gpiochip_init_valid_mask" drops the range and this is not expected behavior.

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andy Shevchenko]

On Mon, Apr 11, 2022 at 5:46 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote:

...

> > What does documentation tell about it?
>
> Documentation (devicetree/bindings/gpio/gpio.txt line 152) tells that
> "This property indicates the start and size of the GPIOs that can't be used."
> And the example (line 178) at the same file shows that the second element of
> a tuple is the count: "gpio-reserved-ranges = <0 4>, <12 2>;"
>
> > Does it need to be fixed?

This question was related to the documentation contents.

> I think so, because the current implementation doesn't reserve some GPIO ranges.
> For example, we have 20 GPIOs and we want to reserve GPIOs from 14 to 19.
> In this case the "reserved-ranges" looks like "<14 6>" but the
> "of_gpiochip_init_valid_mask" drops the range and this is not expected behavior.

On top of that, it would be nice to be sure that at least all current
in-kernel users (meaning all DTS provided so far by the kernel) do
interpret it as start,size. Otherwise this will be an (unacceptable)
ABI change and hence documentation would need to be fixed with
variable names in the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andrei Lalaev]

On Mon, Apr 11, 2022 at 7:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> On top of that, it would be nice to be sure that at least all current
> in-kernel users (meaning all DTS provided so far by the kernel) do
> interpret it as start,size.

I checked the DTS and only 36 of them use "gpio-reserved-ranges".
The 33 of them use tuple with the second element that is less than the first.
So it means that the maintainers interpreted it as "start,size".

And only 3 of them use one tuple with the second element is greater than the first.
The list of this DTS:
    1. arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
        AngeloGioacchino Del Regno added it in the commit 9da65e441d4d
        ("arm64: dts: qcom: Add support for SONY Xperia X Performance / XZ / XZs (msm8996, Tone platform)")
        But in another commit 8b36c824b9a77 ("arm64: dts: qcom: sdm630-xperia-nile: Add all RPM and fixed regulators")
        he clearly interpreted it as "start,size" because the second element
        is less than the first.

    2. arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts
        The same maintainer as for the previous DTS.
        He added "reserved-range" in the commit 122d2c5f31b6e
        ("arm64: dts: qcom: Add support for MSM8998 F(x)tec Pro1 QX1000")

    3. arch/arm64/boot/dts/qcom/sa8155p-adp.dts
        Bhupesh Sharma added it in the commit 12dd4ebda47ab
        ("arm64: dts: qcom: Fix usb entries for SA8155p adp board")

Should I ask these maintainers how they interpreted this property?

> Otherwise this will be an (unacceptable) ABI change and hence documentation
> would need to be fixed with variable names in the code

I want you to notice that "of_gpiochip_init_valid_mask" uses "bitmap_clear"
that clears "nbits" bits starting from the "start". So it can't be interpreted
as "start,end". That's why I think everyone interprets it as "start,size" because
it works like "start,size" and the documentation tells it is "start,size".

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andy Shevchenko]

On Tue, Apr 12, 2022 at 10:13 AM Andrei Lalaev <andrei.lalaev@emlid.com> wrote:
> On Mon, Apr 11, 2022 at 7:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > On top of that, it would be nice to be sure that at least all current
> > in-kernel users (meaning all DTS provided so far by the kernel) do
> > interpret it as start,size.
>
> I checked the DTS and only 36 of them use "gpio-reserved-ranges".
> The 33 of them use tuple with the second element that is less than the first.
> So it means that the maintainers interpreted it as "start,size".
>
> And only 3 of them use one tuple with the second element is greater than the first.
> The list of this DTS:
>     1. arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
>         AngeloGioacchino Del Regno added it in the commit 9da65e441d4d
>         ("arm64: dts: qcom: Add support for SONY Xperia X Performance / XZ / XZs (msm8996, Tone platform)")
>         But in another commit 8b36c824b9a77 ("arm64: dts: qcom: sdm630-xperia-nile: Add all RPM and fixed regulators")
>         he clearly interpreted it as "start,size" because the second element
>         is less than the first.
>
>     2. arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts
>         The same maintainer as for the previous DTS.
>         He added "reserved-range" in the commit 122d2c5f31b6e
>         ("arm64: dts: qcom: Add support for MSM8998 F(x)tec Pro1 QX1000")
>
>     3. arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>         Bhupesh Sharma added it in the commit 12dd4ebda47ab
>         ("arm64: dts: qcom: Fix usb entries for SA8155p adp board")
>
> Should I ask these maintainers how they interpreted this property?

Ideally it would be nice to have their responses. Meanwhile...

> > Otherwise this will be an (unacceptable) ABI change and hence documentation
> > would need to be fixed with variable names in the code
>
> I want you to notice that "of_gpiochip_init_valid_mask" uses "bitmap_clear"
> that clears "nbits" bits starting from the "start". So it can't be interpreted
> as "start,end". That's why I think everyone interprets it as "start,size" because
> it works like "start,size" and the documentation tells it is "start,size".

...meanwhile try to compress above into a few sentences and put it to
the commit message explaining that the de facto use of the values
seems as start,size and questioned DTSes have been asked for an
explanation from the current maintainers.

I'm now 99% confident that your patch is correct.
Thanks for doing all this analysis.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Bartosz Golaszewski]

On Mon, Apr 11, 2022 at 2:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 12:57 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote:
> >
> > Use "greater" instead of "greater or equal" when performs bounds check
> > to make sure that GPIOS are in available range. Previous implementation
>
> the available
>
> > skipped ranges which include last GPIO in range.
>
> the last
>
> Should it have a Fixes tag?
>
> OTOH, the current implementation suggests that we have start,end
> rather than start,count. What does documentation tell about it? Does
> it need to be fixed?
>
> --
> With Best Regards,
> Andy Shevchenko

Thanks Andy, I rushed this one. Backing it out.

Bart

Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andy Shevchenko]

On Tue, Apr 12, 2022 at 11:28 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Apr 11, 2022 at 2:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> > OTOH, the current implementation suggests that we have start,end
> > rather than start,count. What does documentation tell about it? Does
> > it need to be fixed?

> Thanks Andy, I rushed this one. Backing it out.

With the last analysis by Andrei I think the patch is correct. I
suggest waiting for v2 with a better commit message.

-- 
With Best Regards,
Andy Shevchenko