[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Heinrich Schuchardt]

plic_priority_save() and plic_priority_restore() access indexes 1 to num of
the passed array. Avoid a buffer overrun by increasing the used array size
by one.

Addresses-Coverity-ID: 1530251 ("Out-of-bounds access")
Addresses-Coverity-ID: 1530252 ("Out-of-bounds access")
Fixes: 8509e46ca63a ("lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 platform/generic/allwinner/sun20i-d1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
index 1da9e5b..9891ad0 100644
--- a/platform/generic/allwinner/sun20i-d1.c
+++ b/platform/generic/allwinner/sun20i-d1.c
@@ -72,7 +72,7 @@ static void sun20i_d1_csr_restore(void)
 #define PLIC_SOURCES			176
 #define PLIC_IE_WORDS			((PLIC_SOURCES + 31) / 32)
 
-static u8 plic_priority[PLIC_SOURCES];
+static u8 plic_priority[PLIC_SOURCES + 1];
 static u32 plic_sie[PLIC_IE_WORDS];
 static u32 plic_threshold;
 
-- 
2.37.2

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Bin Meng]

On Tue, Dec 27, 2022 at 7:03 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> plic_priority_save() and plic_priority_restore() access indexes 1 to num of
> the passed array. Avoid a buffer overrun by increasing the used array size
> by one.
>
> Addresses-Coverity-ID: 1530251 ("Out-of-bounds access")
> Addresses-Coverity-ID: 1530252 ("Out-of-bounds access")

Where is the Coverity for OpenSBI project hosted?

> Fixes: 8509e46ca63a ("lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  platform/generic/allwinner/sun20i-d1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 1da9e5b..9891ad0 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -72,7 +72,7 @@ static void sun20i_d1_csr_restore(void)
>  #define PLIC_SOURCES                   176
>  #define PLIC_IE_WORDS                  ((PLIC_SOURCES + 31) / 32)
>
> -static u8 plic_priority[PLIC_SOURCES];
> +static u8 plic_priority[PLIC_SOURCES + 1];

This change could fix the Coverity, but IMHO we should not change this
value. PLIC_SOURCES should be 176.

You probably are using a buggy DTB that contains a wrong "riscv,ndev",
which I pointed out in the linux-riscv ML.

>  static u32 plic_sie[PLIC_IE_WORDS];
>  static u32 plic_threshold;
>

My original patch [1] contains a size check against "riscv,ndev" in
the DTB. But somehow that size check got dropped when the patch was
applied.

[1] https://patchwork.ozlabs.org/project/opensbi/patch/20221211065424.806478-2-bmeng at tinylab.org/

Regards,
Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Heinrich Schuchardt]

On 12/27/22 12:46, Bin Meng wrote:
> On Tue, Dec 27, 2022 at 7:03 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> plic_priority_save() and plic_priority_restore() access indexes 1 to num of
>> the passed array. Avoid a buffer overrun by increasing the used array size
>> by one.
>>
>> Addresses-Coverity-ID: 1530251 ("Out-of-bounds access")
>> Addresses-Coverity-ID: 1530252 ("Out-of-bounds access")
> 
> Where is the Coverity for OpenSBI project hosted?

https://scan.coverity.com

> 
>> Fixes: 8509e46ca63a ("lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   platform/generic/allwinner/sun20i-d1.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
>> index 1da9e5b..9891ad0 100644
>> --- a/platform/generic/allwinner/sun20i-d1.c
>> +++ b/platform/generic/allwinner/sun20i-d1.c
>> @@ -72,7 +72,7 @@ static void sun20i_d1_csr_restore(void)
>>   #define PLIC_SOURCES                   176
>>   #define PLIC_IE_WORDS                  ((PLIC_SOURCES + 31) / 32)
>>
>> -static u8 plic_priority[PLIC_SOURCES];
>> +static u8 plic_priority[PLIC_SOURCES + 1];
> 
> This change could fix the Coverity, but IMHO we should not change this
> value. PLIC_SOURCES should be 176.
> 
> You probably are using a buggy DTB that contains a wrong "riscv,ndev",
> which I pointed out in the linux-riscv ML.

The problem does not depend on any device-tree. PLIC_SOURCES is a 
constant that your code uses in

platform/generic/allwinner/sun20i-d1.c:82: 
fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
platform/generic/allwinner/sun20i-d1.c:88: 
fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);

I wonder why plic_priority_save() and plic_priority_restore() start 
indexing at 1 and not at 0. This might deserve a code comment.
> 
>>   static u32 plic_sie[PLIC_IE_WORDS];
>>   static u32 plic_threshold;
>>
> 
> My original patch [1] contains a size check against "riscv,ndev" in
> the DTB. But somehow that size check got dropped when the patch was
> applied.
> 
> [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221211065424.806478-2-bmeng at tinylab.org/

"riscv,ndev" isn't used anywhere in your patch. Just the constant 
PLIC_SOURCES.

Best regards

Heinrich

> 
> Regards,
> Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Bin Meng]

On Tue, Dec 27, 2022 at 8:01 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 12/27/22 12:46, Bin Meng wrote:
> > On Tue, Dec 27, 2022 at 7:03 PM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> plic_priority_save() and plic_priority_restore() access indexes 1 to num of
> >> the passed array. Avoid a buffer overrun by increasing the used array size
> >> by one.
> >>
> >> Addresses-Coverity-ID: 1530251 ("Out-of-bounds access")
> >> Addresses-Coverity-ID: 1530252 ("Out-of-bounds access")
> >
> > Where is the Coverity for OpenSBI project hosted?
>
> https://scan.coverity.com
>
> >
> >> Fixes: 8509e46ca63a ("lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers")
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   platform/generic/allwinner/sun20i-d1.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> >> index 1da9e5b..9891ad0 100644
> >> --- a/platform/generic/allwinner/sun20i-d1.c
> >> +++ b/platform/generic/allwinner/sun20i-d1.c
> >> @@ -72,7 +72,7 @@ static void sun20i_d1_csr_restore(void)
> >>   #define PLIC_SOURCES                   176
> >>   #define PLIC_IE_WORDS                  ((PLIC_SOURCES + 31) / 32)
> >>
> >> -static u8 plic_priority[PLIC_SOURCES];
> >> +static u8 plic_priority[PLIC_SOURCES + 1];
> >
> > This change could fix the Coverity, but IMHO we should not change this
> > value. PLIC_SOURCES should be 176.
> >
> > You probably are using a buggy DTB that contains a wrong "riscv,ndev",
> > which I pointed out in the linux-riscv ML.
>
> The problem does not depend on any device-tree. PLIC_SOURCES is a
> constant that your code uses in
>
> platform/generic/allwinner/sun20i-d1.c:82:
> fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
> platform/generic/allwinner/sun20i-d1.c:88:
> fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>
> I wonder why plic_priority_save() and plic_priority_restore() start
> indexing at 1 and not at 0. This might deserve a code comment.

Please see the link I provided. In my original patch with the size
check, this Coverity issue won't happen.

> >
> >>   static u32 plic_sie[PLIC_IE_WORDS];
> >>   static u32 plic_threshold;
> >>
> >
> > My original patch [1] contains a size check against "riscv,ndev" in
> > the DTB. But somehow that size check got dropped when the patch was
> > applied.
> >
> > [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221211065424.806478-2-bmeng at tinylab.org/
>
> "riscv,ndev" isn't used anywhere in your patch. Just the constant
> PLIC_SOURCES.
>

Changing the array size to PLIC_SOURCES + 1 does not make sense. The
PLIC_SOURCES should be 176 which is correct as it includes source 0 on
the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
will create a buffer overrun.

[1] https://lore.kernel.org/linux-riscv/20221128133421.58614-1-bmeng at tinylab.org/

Regards,
Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Bin Meng wrote:

> Changing the array size to PLIC_SOURCES + 1 does not make sense. The
> PLIC_SOURCES should be 176 which is correct as it includes source 0 on
> the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
> will create a buffer overrun.

The range check will always allow that overrrun.

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Bin Meng]

On Tue, Dec 27, 2022 at 9:04 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 27 2022, Bin Meng wrote:
>
> > Changing the array size to PLIC_SOURCES + 1 does not make sense. The
> > PLIC_SOURCES should be 176 which is correct as it includes source 0 on
> > the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
> > will create a buffer overrun.
>
> The range check will always allow that overrrun.
>

Well, with a correct dtb it doesn't.

Strictly speaking, your proposed fix allows that overrun too, if the
given num is an arbitrary larger value than the size of the priority
array.

Regards,
Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Bin Meng wrote:

> On Tue, Dec 27, 2022 at 9:04 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 27 2022, Bin Meng wrote:
>>
>> > Changing the array size to PLIC_SOURCES + 1 does not make sense. The
>> > PLIC_SOURCES should be 176 which is correct as it includes source 0 on
>> > the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
>> > will create a buffer overrun.
>>
>> The range check will always allow that overrrun.
>>
>
> Well, with a correct dtb it doesn't.

You cannot argue with the dtb, since this is externally controlled.

> Strictly speaking, your proposed fix allows that overrun too

How?

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Bin Meng]

On Tue, Dec 27, 2022 at 9:36 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 27 2022, Bin Meng wrote:
>
> > On Tue, Dec 27, 2022 at 9:04 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >> On Dez 27 2022, Bin Meng wrote:
> >>
> >> > Changing the array size to PLIC_SOURCES + 1 does not make sense. The
> >> > PLIC_SOURCES should be 176 which is correct as it includes source 0 on
> >> > the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
> >> > will create a buffer overrun.
> >>
> >> The range check will always allow that overrrun.
> >>
> >
> > Well, with a correct dtb it doesn't.
>
> You cannot argue with the dtb, since this is externally controlled.
>
> > Strictly speaking, your proposed fix allows that overrun too
>
> How?
>

Passing num = 200 to plic_priority_save/restore.

You can argue my example is a misuse of the API, but I can argue a
wrong dtb should not be in the first place too.

Regards,
Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Bin Meng wrote:

> Passing num = 200 to plic_priority_save/restore.
>
> You can argue my example is a misuse of the API,

It is a bug just like the bug in plic_priority_save/restore.

> but I can argue a wrong dtb should not be in the first place too.

The dtb is an external resource.

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Samuel Holland]

On 12/27/22 07:13, Bin Meng wrote:
> On Tue, Dec 27, 2022 at 9:04 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 27 2022, Bin Meng wrote:
>>
>>> Changing the array size to PLIC_SOURCES + 1 does not make sense. The
>>> PLIC_SOURCES should be 176 which is correct as it includes source 0 on
>>> the Allwinner SoC. The "riscv,ndev" [1] should not be 176 otherwise it
>>> will create a buffer overrun.
>>
>> The range check will always allow that overrrun.
>>
> 
> Well, with a correct dtb it doesn't.

No, the bug has nothing at all to do with the DTB. The bug is purely in
the C code.

> Strictly speaking, your proposed fix allows that overrun too, if the
> given num is an arbitrary larger value than the size of the priority
> array.

This is already what you are doing! You have:

	static u8 plic_priority[PLIC_SOURCES];

and

	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);

The combination of those two lines is necessarily incorrect. Because you
dereference priority[num], the number of elements in the array *must* be
greater than num.

Since the range of external interrupt sources is [1, 175], the
definitions need to be:

#define PLIC_SOURCES                    175
#define PLIC_IE_WORDS                   ((PLIC_SOURCES / 32) + 1)

static u8 plic_priority[1 + PLIC_SOURCES];

Anything else is either wrong or wasting space.

Regards,
Samuel

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Samuel Holland wrote:

> Anything else is either wrong or wasting space.

Not wasting space:

diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index d633514..901ffaa 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -39,14 +39,14 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
 void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
 {
 	for (u32 i = 1; i <= num; i++)
-		priority[i] = plic_get_priority(plic, i);
+		priority[i - 1] = plic_get_priority(plic, i);
 }
 
 void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
 			   u32 num)
 {
 	for (u32 i = 1; i <= num; i++)
-		plic_set_priority(plic, i, priority[i]);
+		plic_set_priority(plic, i, priority[i - 1]);
 }
 
 static u32 plic_get_thresh(const struct plic_data *plic, u32 cntxid)

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Samuel Holland]

On 12/27/22 12:39, Andreas Schwab wrote:
> On Dez 27 2022, Samuel Holland wrote:
> 
>> Anything else is either wrong or wasting space.
> 
> Not wasting space:

I am aware. However, this sort of change is not feasible for
plic_context_save()/plic_context_restore(). It would severely complicate
those functions with extra bit manipulation (thus wasting even more
space in code size). So I am happy to pay one byte here for consistency
between the two sets of functions.

Regards,
Samuel

> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index d633514..901ffaa 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -39,14 +39,14 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
>  void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
>  {
>  	for (u32 i = 1; i <= num; i++)
> -		priority[i] = plic_get_priority(plic, i);
> +		priority[i - 1] = plic_get_priority(plic, i);
>  }
>  
>  void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
>  			   u32 num)
>  {
>  	for (u32 i = 1; i <= num; i++)
> -		plic_set_priority(plic, i, priority[i]);
> +		plic_set_priority(plic, i, priority[i - 1]);
>  }
>  
>  static u32 plic_get_thresh(const struct plic_data *plic, u32 cntxid)
> 

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Samuel Holland wrote:

> I am aware. However, this sort of change is not feasible for
> plic_context_save()/plic_context_restore(). It would severely complicate
> those functions with extra bit manipulation (thus wasting even more
> space in code size).

If your compiler is so dumb you have a much bigger problem.

-- 
Andreas Schwab, schwab at linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Samuel Holland]

On 12/27/22 12:52, Andreas Schwab wrote:
> On Dez 27 2022, Samuel Holland wrote:
> 
>> I am aware. However, this sort of change is not feasible for
>> plic_context_save()/plic_context_restore(). It would severely complicate
>> those functions with extra bit manipulation (thus wasting even more
>> space in code size).
> 
> If your compiler is so dumb you have a much bigger problem.

I'm disappointed that I even have to explain this...

"This sort of change" to avoid using space for IRQ 0, thus saving one
*bit* in the array used by plic_context_save(), would require combining
two registers into one array element with shifts:

	enable[i] = plic_get_ie(plic, context_id, i) >> 1 |
	            plic_get_ie(plic, context_id, i + 1) << 31;

and doing the opposite transformation in plic_context_restore(). The
compiler is not going to optimize that out.

If you really care about shaving off bytes, there is plenty of
low-hanging fruit elsewhere in OpenSBI. This bikeshed really does not
need painting.

Regards,
Samuel

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

On Dez 27 2022, Samuel Holland wrote:

> "This sort of change" to avoid using space for IRQ 0, thus saving one
> *bit* in the array

??? *byte*

> used by plic_context_save(),

??? plic_priority_save/restore

> 	enable[i] = plic_get_ie(plic, context_id, i) >> 1 |
> 	            plic_get_ie(plic, context_id, i + 1) << 31;

??? Nothing at all changes here.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Andreas Schwab]

The actual bug is in plic_priority_save/restore.

diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index d633514..901ffaa 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -39,14 +39,14 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
 void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
 {
 	for (u32 i = 1; i <= num; i++)
-		priority[i] = plic_get_priority(plic, i);
+		priority[i - 1] = plic_get_priority(plic, i);
 }
 
 void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
 			   u32 num)
 {
 	for (u32 i = 1; i <= num; i++)
-		plic_set_priority(plic, i, priority[i]);
+		plic_set_priority(plic, i, priority[i - 1]);
 }
 
 static u32 plic_get_thresh(const struct plic_data *plic, u32 cntxid)

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Heinrich Schuchardt]

On 12/27/22 13:05, Andreas Schwab wrote:
> The actual bug is in plic_priority_save/restore.


The problem really starts at include/sbi_utils/irqchip/plic.h where 
there is no comment at all describing the usage of the function and its 
parameters. So we don't know what the different authors of these 
functions intended to use index 0 for.

It would be great if we could move the project to follow the kernel 
documentation style (https://docs.kernel.org/doc-guide/kernel-doc.html)


> 
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index d633514..901ffaa 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -39,14 +39,14 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
>   void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
>   {
>   	for (u32 i = 1; i <= num; i++)

In patch 34da6638 ("lib: utils/irqchip: plic: Fix the off-by-one error 
in priority save/restore helpers") Bin wrote

Interrupt source 0 is reserved. Hence the irq should start from 1.

Why was the the upper limit changed?

-       for (u32 i = 0; i < plic->num_src; i++)
+       for (u32 i = 1; i <= plic->num_src; i++)

Best regards

Heinrich

> -		priority[i] = plic_get_priority(plic, i);
> +		priority[i - 1] = plic_get_priority(plic, i);
>   }
>   
>   void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
>   			   u32 num)
>   {
>   	for (u32 i = 1; i <= num; i++)
> -		plic_set_priority(plic, i, priority[i]);
> +		plic_set_priority(plic, i, priority[i - 1]);
>   }
>   
>   static u32 plic_get_thresh(const struct plic_data *plic, u32 cntxid)
> 

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Bin Meng]

On Tue, Dec 27, 2022 at 8:30 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 12/27/22 13:05, Andreas Schwab wrote:
> > The actual bug is in plic_priority_save/restore.
>
>
> The problem really starts at include/sbi_utils/irqchip/plic.h where
> there is no comment at all describing the usage of the function and its
> parameters. So we don't know what the different authors of these
> functions intended to use index 0 for.
>
> It would be great if we could move the project to follow the kernel
> documentation style (https://docs.kernel.org/doc-guide/kernel-doc.html)
>
>
> >
> > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> > index d633514..901ffaa 100644
> > --- a/lib/utils/irqchip/plic.c
> > +++ b/lib/utils/irqchip/plic.c
> > @@ -39,14 +39,14 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
> >   void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
> >   {
> >       for (u32 i = 1; i <= num; i++)
>
> In patch 34da6638 ("lib: utils/irqchip: plic: Fix the off-by-one error
> in priority save/restore helpers") Bin wrote
>
> Interrupt source 0 is reserved. Hence the irq should start from 1.
>
> Why was the the upper limit changed?
>

Both lower and upper limits are changed because the irq number range
is really [1, plic->num_src].

Previously the author had a wrong understanding of the irq number
hence that's what the patch 34da6638 tried to fix.

Regards,
Bin

[PATCH 1/1] platform: generic: allwinner: avoid buffer overrun

[Anup Patel]

Hi Heinrich,

On Tue, Dec 27, 2022 at 4:31 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> plic_priority_save() and plic_priority_restore() access indexes 1 to num of
> the passed array. Avoid a buffer overrun by increasing the used array size
> by one.
>
> Addresses-Coverity-ID: 1530251 ("Out-of-bounds access")
> Addresses-Coverity-ID: 1530252 ("Out-of-bounds access")
> Fixes: 8509e46ca63a ("lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Do you plan to send a v2 for this ?

Regards,
Anup

> ---
>  platform/generic/allwinner/sun20i-d1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 1da9e5b..9891ad0 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -72,7 +72,7 @@ static void sun20i_d1_csr_restore(void)
>  #define PLIC_SOURCES                   176
>  #define PLIC_IE_WORDS                  ((PLIC_SOURCES + 31) / 32)
>
> -static u8 plic_priority[PLIC_SOURCES];
> +static u8 plic_priority[PLIC_SOURCES + 1];
>  static u32 plic_sie[PLIC_IE_WORDS];
>  static u32 plic_threshold;
>
> --
> 2.37.2
>