Mailing List Archive

On Thu, Mar 28, 2024 at 12:10:21PM +0300, Aleksandr Mishin wrote:
> In davinci_gpio_probe() accessing an element of array 'chips->regs' of size 5 and
> array 'offset_array' of size 5 can lead to a buffer overflow, since the index
> 'bank' can have an out of range value 63.
^^

Where does this 63 come from? SVACE is a static analysis tool. I would
have thought a static checker would say that 'bank' goes up to
UINT_MAX / 32.

This stuff comes from device tree though, so it looks fine to me.

Documentation/devicetree/bindings/gpio/gpio-davinci.yaml: ti,ngpio = <144>;
Documentation/devicetree/bindings/gpio/gpio-davinci.yaml: ti,ngpio = <32>;
Documentation/devicetree/bindings/gpio/gpio-davinci.yaml: ti,ngpio = <56>;
arch/arm/boot/dts/ti/davinci/da850.dtsi: ti,ngpio = <144>;

So it's fine.

I'm not the maintainer of this file so I don't know if adding a sanity
check makes sense but if we wanted to do that we'd have to add it to
davinci_gpio_get_pdata(). Otherwise it would have already had a buffer
overflow earlier in the probe function when we do:

drivers/gpio/gpio-davinci.c
223 if (pdata->gpio_unbanked)
224 nirq = pdata->gpio_unbanked;
225 else
226 nirq = DIV_ROUND_UP(ngpio, 16);
227
228 chips = devm_kzalloc(dev, sizeof(*chips), GFP_KERNEL);
229 if (!chips)
230 return -ENOMEM;
231
232 gpio_base = devm_platform_ioremap_resource(pdev, 0);
233 if (IS_ERR(gpio_base))
234 return PTR_ERR(gpio_base);
235
236 for (i = 0; i < nirq; i++) {
237 chips->irqs[i] = platform_get_irq(pdev, i);
^^^

238 if (chips->irqs[i] < 0)
239 return chips->irqs[i];
240 }

regards,
dan carpenter

From: Aleksandr Mishin <amishin@t-argos.ru>
Date: Thu, 28 Mar 2024 12:10:21 +0300

> In davinci_gpio_probe() accessing an element of array 'chips->regs' of size 5 and
> array 'offset_array' of size 5 can lead to a buffer overflow, since the index
> 'bank' can have an out of range value 63.
> Fix this bug by limiting top index value.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c809e37a3b5a ("gpio: davinci: Allocate the correct amount of memory for controller")
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
> ---
> drivers/gpio/gpio-davinci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index bb499e362912..b65df1f2b83f 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -257,6 +257,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> spin_lock_init(&chips->lock);
>
> nbank = DIV_ROUND_UP(ngpio, 32);
> + if (nbank > MAX_REGS_BANKS || nbank > 5) {
> + nbank = MAX_REGS_BANKS < 5 ? MAX_REGS_BANKS : 5;
> + }

Static analysis warnings make no sense until you provide a reliable way
to trigger the problem on real systems.

> for (bank = 0; bank < nbank; bank++)
> chips->regs[bank] = gpio_base + offset_array[bank];
>

Thanks,
Olek

28.03.2024 18:27, Alexander Lobakin ?????:
> From: Aleksandr Mishin <amishin@t-argos.ru>
> Date: Thu, 28 Mar 2024 12:10:21 +0300
>
>> In davinci_gpio_probe() accessing an element of array 'chips->regs' of size 5 and
>> array 'offset_array' of size 5 can lead to a buffer overflow, since the index
>> 'bank' can have an out of range value 63.
>> Fix this bug by limiting top index value.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: c809e37a3b5a ("gpio: davinci: Allocate the correct amount of memory for controller")
>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
>> ---
>> drivers/gpio/gpio-davinci.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index bb499e362912..b65df1f2b83f 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -257,6 +257,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>> spin_lock_init(&chips->lock);
>>
>> nbank = DIV_ROUND_UP(ngpio, 32);
>> + if (nbank > MAX_REGS_BANKS || nbank > 5) {
>> + nbank = MAX_REGS_BANKS < 5 ? MAX_REGS_BANKS : 5;
>> + }
>
> Static analysis warnings make no sense until you provide a reliable way
> to trigger the problem on real systems.
>
>> for (bank = 0; bank < nbank; bank++)
>> chips->regs[bank] = gpio_base + offset_array[bank];
>>
>
> Thanks,
> Olek
>

I can only see the code at this time. And I see the following:
1. In some configurations CONFIG_ARCH_NR_GPIO value is 2048. So nbank
value can be 64.
2. Previously, a patch was proposed that removes restrictions on the
number of GPIOs
(https://lore.kernel.org/all/cb540a9d73cad36d288664f8b275c6308a4a3168.1662116601.git.christophe.leroy@csgroup.eu/).

--
Kind regards
Aleksandr

From: Aleksandr Mishin <amishin@t-argos.ru>
Date: Thu, 28 Mar 2024 19:23:56 +0300

>
>
> 28.03.2024 18:27, Alexander Lobakin ?????:
>> From: Aleksandr Mishin <amishin@t-argos.ru>
>> Date: Thu, 28 Mar 2024 12:10:21 +0300
>>
>>> In davinci_gpio_probe() accessing an element of array 'chips->regs'
>>> of size 5 and
>>> array 'offset_array' of size 5 can lead to a buffer overflow, since
>>> the index
>>> 'bank' can have an out of range value 63.
>>> Fix this bug by limiting top index value.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Fixes: c809e37a3b5a ("gpio: davinci: Allocate the correct amount of
>>> memory for controller")
>>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
>>> ---
>>>   drivers/gpio/gpio-davinci.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>>> index bb499e362912..b65df1f2b83f 100644
>>> --- a/drivers/gpio/gpio-davinci.c
>>> +++ b/drivers/gpio/gpio-davinci.c
>>> @@ -257,6 +257,9 @@ static int davinci_gpio_probe(struct
>>> platform_device *pdev)
>>>       spin_lock_init(&chips->lock);
>>>         nbank = DIV_ROUND_UP(ngpio, 32);
>>> +    if (nbank > MAX_REGS_BANKS || nbank > 5) {
>>> +        nbank = MAX_REGS_BANKS < 5 ? MAX_REGS_BANKS : 5;
>>> +    }
>>
>> Static analysis warnings make no sense until you provide a reliable way
>> to trigger the problem on real systems.
>>
>>>       for (bank = 0; bank < nbank; bank++)
>>>           chips->regs[bank] = gpio_base + offset_array[bank];
>>>  
>>
>> Thanks,
>> Olek
>>
>
> I can only see the code at this time. And I see the following:
> 1. In some configurations CONFIG_ARCH_NR_GPIO value is 2048. So nbank
> value can be 64.
> 2. Previously, a patch was proposed that removes restrictions on the
> number of GPIOs
> (https://lore.kernel.org/all/cb540a9d73cad36d288664f8b275c6308a4a3168.1662116601.git.christophe.leroy@csgroup.eu/).
>

But no real hardware / device tree which declares such huge number of
GPIOs, right?

CONFIG_ARCH_NR_GPIO is architecture-specific. Davinci is a platform, not
an architecture. If no Davinci board can have a number that would
overflow, the fix doesn't make sense.

Thanks,
Olek

On Thu, Mar 28, 2024 at 04:27:24PM +0100, Alexander Lobakin wrote:
> > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> > index bb499e362912..b65df1f2b83f 100644
> > --- a/drivers/gpio/gpio-davinci.c
> > +++ b/drivers/gpio/gpio-davinci.c
> > @@ -257,6 +257,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> > spin_lock_init(&chips->lock);
> >
> > nbank = DIV_ROUND_UP(ngpio, 32);
> > + if (nbank > MAX_REGS_BANKS || nbank > 5) {
> > + nbank = MAX_REGS_BANKS < 5 ? MAX_REGS_BANKS : 5;
> > + }
>
> Static analysis warnings make no sense until you provide a reliable way
> to trigger the problem on real systems.

This patch isn't correct, but we merge a few static checker fixes every
day.

regards,
dan carpenter