[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Bin Meng]

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

Fixes: 2b79b694a805 ("lib: irqchip/plic: Add priority save/restore helpers")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Anup Patel <anup@brainfault.org>
---

(no changes since v1)

 lib/utils/irqchip/plic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index 73d7788..4df9020 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -38,13 +38,13 @@ 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)
 {
-	for (u32 i = 0; i < plic->num_src; i++)
+	for (u32 i = 1; i <= plic->num_src; i++)
 		priority[i] = plic_get_priority(plic, i);
 }
 
 void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
 {
-	for (u32 i = 0; i < plic->num_src; i++)
+	for (u32 i = 1; i <= plic->num_src; i++)
 		plic_set_priority(plic, i, priority[i]);
 }
 
-- 
2.34.1

[PATCH v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Bin Meng]

Currently the priority save/restore helpers writes/reads the provided
array using an index whose maximum value is determined by PLIC, which
potentially may disagree with the caller to these helpers.

Add a parameter to ask the caller to provide the size limit of the
array to ensure no out-of-bound access happens.

Signed-off-by: Bin Meng <bmeng@tinylab.org>

---

Changes in v3:
- fix the size limit check
- move the size limit check to plic_priority_save/restore
- add parameter description to fdt_plic_priority_save/restore

Changes in v2:
- new patch: libs: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

 include/sbi_utils/irqchip/fdt_irqchip_plic.h | 14 ++++++++++++--
 include/sbi_utils/irqchip/plic.h             |  5 +++--
 lib/utils/irqchip/fdt_irqchip_plic.c         |  8 ++++----
 lib/utils/irqchip/plic.c                     | 15 +++++++++++----
 platform/generic/allwinner/sun20i-d1.c       |  4 ++--
 5 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
index 98d4de5..d5b1c60 100644
--- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
+++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
@@ -9,9 +9,19 @@
 
 #include <sbi/sbi_types.h>
 
-void fdt_plic_priority_save(u8 *priority);
+/**
+ * Save the PLIC priority state
+ * @param priority pointer to the memory region for the saved priority
+ * @param num size of the memory region including interrupt source 0
+ */
+void fdt_plic_priority_save(u8 *priority, u32 num);
 
-void fdt_plic_priority_restore(const u8 *priority);
+/**
+ * Restore the PLIC priority state
+ * @param priority pointer to the memory region for the saved priority
+ * @param num size of the memory region including interrupt source 0
+ */
+void fdt_plic_priority_restore(const u8 *priority, u32 num);
 
 void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
 
diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
index 48c24f0..38704a1 100644
--- a/include/sbi_utils/irqchip/plic.h
+++ b/include/sbi_utils/irqchip/plic.h
@@ -18,9 +18,10 @@ struct plic_data {
 };
 
 /* So far, priorities on all consumers of these functions fit in 8 bits. */
-void plic_priority_save(const struct plic_data *plic, u8 *priority);
+void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num);
 
-void plic_priority_restore(const struct plic_data *plic, const u8 *priority);
+void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
+			   u32 num);
 
 void plic_context_save(const struct plic_data *plic, int context_id,
 		       u32 *enable, u32 *threshold);
diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
index fe08836..7d76c5b 100644
--- a/lib/utils/irqchip/fdt_irqchip_plic.c
+++ b/lib/utils/irqchip/fdt_irqchip_plic.c
@@ -24,18 +24,18 @@ static struct plic_data plic[PLIC_MAX_NR];
 static struct plic_data *plic_hartid2data[SBI_HARTMASK_MAX_BITS];
 static int plic_hartid2context[SBI_HARTMASK_MAX_BITS][2];
 
-void fdt_plic_priority_save(u8 *priority)
+void fdt_plic_priority_save(u8 *priority, u32 num)
 {
 	struct plic_data *plic = plic_hartid2data[current_hartid()];
 
-	plic_priority_save(plic, priority);
+	plic_priority_save(plic, priority, num);
 }
 
-void fdt_plic_priority_restore(const u8 *priority)
+void fdt_plic_priority_restore(const u8 *priority, u32 num)
 {
 	struct plic_data *plic = plic_hartid2data[current_hartid()];
 
-	plic_priority_restore(plic, priority);
+	plic_priority_restore(plic, priority, num);
 }
 
 void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index 4df9020..dca5678 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -36,15 +36,22 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
 	writel(val, plic_priority);
 }
 
-void plic_priority_save(const struct plic_data *plic, u8 *priority)
+void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
 {
-	for (u32 i = 1; i <= plic->num_src; i++)
+	if (num > plic->num_src)
+		num = plic->num_src;
+
+	for (u32 i = 1; i <= num; i++)
 		priority[i] = plic_get_priority(plic, i);
 }
 
-void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
+void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
+			   u32 num)
 {
-	for (u32 i = 1; i <= plic->num_src; i++)
+	if (num > plic->num_src)
+		num = plic->num_src;
+
+	for (u32 i = 1; i <= num; i++)
 		plic_set_priority(plic, i, priority[i]);
 }
 
diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
index 18d330d..1f27575 100644
--- a/platform/generic/allwinner/sun20i-d1.c
+++ b/platform/generic/allwinner/sun20i-d1.c
@@ -79,13 +79,13 @@ static u32 plic_threshold;
 static void sun20i_d1_plic_save(void)
 {
 	fdt_plic_context_save(true, plic_sie, &plic_threshold);
-	fdt_plic_priority_save(plic_priority);
+	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
 }
 
 static void sun20i_d1_plic_restore(void)
 {
 	thead_plic_restore();
-	fdt_plic_priority_restore(plic_priority);
+	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
 	fdt_plic_context_restore(true, plic_sie, plic_threshold);
 }
 
-- 
2.34.1

[PATCH v3 3/5] lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()

[Bin Meng]

The number of interrupt enable register in words was once correct,
but was wrongly changed to have an off-by-one error since
commit 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function").

Fixes: 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Anup Patel <anup@brainfault.org>

---

(no changes since v2)

Changes in v2:
- new patch: lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()

 lib/utils/irqchip/plic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index dca5678..568f814 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -127,7 +127,7 @@ int plic_context_init(const struct plic_data *plic, int context_id,
 	if (!plic || context_id < 0)
 		return SBI_EINVAL;
 
-	ie_words = (plic->num_src + 31) / 32;
+	ie_words = plic->num_src / 32 + 1;
 	ie_value = enable ? 0xffffffffU : 0U;
 
 	for (u32 i = 0; i < ie_words; i++)
-- 
2.34.1

[PATCH v3 4/5] lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers

[Bin Meng]

plic->num_src holds the number of interrupt sources without interrupt
source 0 but the interrupt enable register includes a bit for the
interrupt source 0 in the first word.

Fixes: 415ecf28f7ad ("lib: irqchip/plic: Add context save/restore helpers")
Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Anup Patel <anup@brainfault.org>

---

(no changes since v2)

Changes in v2:
- new patch: lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers

 lib/utils/irqchip/plic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index 568f814..b152bb7 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -100,7 +100,7 @@ static void plic_set_ie(const struct plic_data *plic, u32 cntxid,
 void plic_context_save(const struct plic_data *plic, int context_id,
 		       u32 *enable, u32 *threshold)
 {
-	u32 ie_words = (plic->num_src + 31) / 32;
+	u32 ie_words = plic->num_src / 32 + 1;
 
 	for (u32 i = 0; i < ie_words; i++)
 		enable[i] = plic_get_ie(plic, context_id, i);
@@ -111,7 +111,7 @@ void plic_context_save(const struct plic_data *plic, int context_id,
 void plic_context_restore(const struct plic_data *plic, int context_id,
 			  const u32 *enable, u32 threshold)
 {
-	u32 ie_words = (plic->num_src + 31) / 32;
+	u32 ie_words = plic->num_src / 32 + 1;
 
 	for (u32 i = 0; i < ie_words; i++)
 		plic_set_ie(plic, context_id, i, enable[i]);
-- 
2.34.1

[PATCH v3 5/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

[Bin Meng]

Currently the context save/restore helpers writes/reads the provided
array using an index whose maximum value is determined by PLIC, which
potentially may disagree with the caller to these helpers.

Add a parameter to ask the caller to provide the size limit of the
array to ensure no out-of-bound access happens.

Signed-off-by: Bin Meng <bmeng@tinylab.org>

---

Changes in v3:
- move the size limit check to plic_context_save/restore

Changes in v2:
- new patch: lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

 include/sbi_utils/irqchip/fdt_irqchip_plic.h |  5 +++--
 include/sbi_utils/irqchip/plic.h             |  4 ++--
 lib/utils/irqchip/fdt_irqchip_plic.c         |  9 +++++----
 lib/utils/irqchip/plic.c                     | 14 ++++++++++----
 platform/generic/allwinner/sun20i-d1.c       |  5 +++--
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
index d5b1c60..df645dd 100644
--- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
+++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
@@ -23,9 +23,10 @@ void fdt_plic_priority_save(u8 *priority, u32 num);
  */
 void fdt_plic_priority_restore(const u8 *priority, u32 num);
 
-void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
+void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num);
 
-void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold);
+void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
+			      u32 num);
 
 void thead_plic_restore(void);
 
diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
index 38704a1..112a714 100644
--- a/include/sbi_utils/irqchip/plic.h
+++ b/include/sbi_utils/irqchip/plic.h
@@ -24,10 +24,10 @@ void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
 			   u32 num);
 
 void plic_context_save(const struct plic_data *plic, int context_id,
-		       u32 *enable, u32 *threshold);
+		       u32 *enable, u32 *threshold, u32 num);
 
 void plic_context_restore(const struct plic_data *plic, int context_id,
-			  const u32 *enable, u32 threshold);
+			  const u32 *enable, u32 threshold, u32 num);
 
 int plic_context_init(const struct plic_data *plic, int context_id,
 		      bool enable, u32 threshold);
diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
index 7d76c5b..c54f45c 100644
--- a/lib/utils/irqchip/fdt_irqchip_plic.c
+++ b/lib/utils/irqchip/fdt_irqchip_plic.c
@@ -38,22 +38,23 @@ void fdt_plic_priority_restore(const u8 *priority, u32 num)
 	plic_priority_restore(plic, priority, num);
 }
 
-void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
+void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num)
 {
 	u32 hartid = current_hartid();
 
 	plic_context_save(plic_hartid2data[hartid],
 			  plic_hartid2context[hartid][smode],
-			  enable, threshold);
+			  enable, threshold, num);
 }
 
-void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold)
+void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
+			      u32 num)
 {
 	u32 hartid = current_hartid();
 
 	plic_context_restore(plic_hartid2data[hartid],
 			     plic_hartid2context[hartid][smode],
-			     enable, threshold);
+			     enable, threshold, num);
 }
 
 static int irqchip_plic_warm_init(void)
diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
index b152bb7..8089a0b 100644
--- a/lib/utils/irqchip/plic.c
+++ b/lib/utils/irqchip/plic.c
@@ -98,22 +98,28 @@ static void plic_set_ie(const struct plic_data *plic, u32 cntxid,
 }
 
 void plic_context_save(const struct plic_data *plic, int context_id,
-		       u32 *enable, u32 *threshold)
+		       u32 *enable, u32 *threshold, u32 num)
 {
 	u32 ie_words = plic->num_src / 32 + 1;
 
-	for (u32 i = 0; i < ie_words; i++)
+	if (num > ie_words)
+		num = ie_words;
+
+	for (u32 i = 0; i < num; i++)
 		enable[i] = plic_get_ie(plic, context_id, i);
 
 	*threshold = plic_get_thresh(plic, context_id);
 }
 
 void plic_context_restore(const struct plic_data *plic, int context_id,
-			  const u32 *enable, u32 threshold)
+			  const u32 *enable, u32 threshold, u32 num)
 {
 	u32 ie_words = plic->num_src / 32 + 1;
 
-	for (u32 i = 0; i < ie_words; i++)
+	if (num > ie_words)
+		num = ie_words;
+
+	for (u32 i = 0; i < num; i++)
 		plic_set_ie(plic, context_id, i, enable[i]);
 
 	plic_set_thresh(plic, context_id, threshold);
diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
index 1f27575..1da9e5b 100644
--- a/platform/generic/allwinner/sun20i-d1.c
+++ b/platform/generic/allwinner/sun20i-d1.c
@@ -78,7 +78,7 @@ static u32 plic_threshold;
 
 static void sun20i_d1_plic_save(void)
 {
-	fdt_plic_context_save(true, plic_sie, &plic_threshold);
+	fdt_plic_context_save(true, plic_sie, &plic_threshold, PLIC_IE_WORDS);
 	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
 }
 
@@ -86,7 +86,8 @@ static void sun20i_d1_plic_restore(void)
 {
 	thead_plic_restore();
 	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
-	fdt_plic_context_restore(true, plic_sie, plic_threshold);
+	fdt_plic_context_restore(true, plic_sie, plic_threshold,
+				 PLIC_IE_WORDS);
 }
 
 /*
-- 
2.34.1

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Andreas Schwab]

On Dez 11 2022, Bin Meng wrote:

> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 73d7788..4df9020 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -38,13 +38,13 @@ 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)
>  {
> -	for (u32 i = 0; i < plic->num_src; i++)
> +	for (u32 i = 1; i <= plic->num_src; i++)
>  		priority[i] = plic_get_priority(plic, i);

That needs to adjust the index into the priority array.

>  void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
>  {
> -	for (u32 i = 0; i < plic->num_src; i++)
> +	for (u32 i = 1; i <= plic->num_src; i++)
>  		plic_set_priority(plic, i, priority[i]);

Likewise.

-- 
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 v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Bin Meng]

On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 11 2022, Bin Meng wrote:
>
> > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> > index 73d7788..4df9020 100644
> > --- a/lib/utils/irqchip/plic.c
> > +++ b/lib/utils/irqchip/plic.c
> > @@ -38,13 +38,13 @@ 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)
> >  {
> > -     for (u32 i = 0; i < plic->num_src; i++)
> > +     for (u32 i = 1; i <= plic->num_src; i++)
> >               priority[i] = plic_get_priority(plic, i);
>
> That needs to adjust the index into the priority array.

Does that help anything? It just confuses people more.

I added function parameter descriptions in patch 2 to make it crystal
clear, that the priority array needs to include interrupt source 0.

>
> >  void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
> >  {
> > -     for (u32 i = 0; i < plic->num_src; i++)
> > +     for (u32 i = 1; i <= plic->num_src; i++)
> >               plic_set_priority(plic, i, priority[i]);
>
> Likewise.
>

Regards,
Bin

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Andreas Schwab]

On Dez 11 2022, Bin Meng wrote:

> On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 11 2022, Bin Meng wrote:
>>
>> > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
>> > index 73d7788..4df9020 100644
>> > --- a/lib/utils/irqchip/plic.c
>> > +++ b/lib/utils/irqchip/plic.c
>> > @@ -38,13 +38,13 @@ 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)
>> >  {
>> > -     for (u32 i = 0; i < plic->num_src; i++)
>> > +     for (u32 i = 1; i <= plic->num_src; i++)
>> >               priority[i] = plic_get_priority(plic, i);
>>
>> That needs to adjust the index into the priority array.
>
> Does that help anything?

Yes, it fixes an off-by-one error.

-- 
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 v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:25 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Currently the priority save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
>
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

>
> ---
>
> Changes in v3:
> - fix the size limit check
> - move the size limit check to plic_priority_save/restore
> - add parameter description to fdt_plic_priority_save/restore
>
> Changes in v2:
> - new patch: libs: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers
>
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h | 14 ++++++++++++--
>  include/sbi_utils/irqchip/plic.h             |  5 +++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  8 ++++----
>  lib/utils/irqchip/plic.c                     | 15 +++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  4 ++--
>  5 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> index 98d4de5..d5b1c60 100644
> --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> @@ -9,9 +9,19 @@
>
>  #include <sbi/sbi_types.h>
>
> -void fdt_plic_priority_save(u8 *priority);
> +/**
> + * Save the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_save(u8 *priority, u32 num);
>
> -void fdt_plic_priority_restore(const u8 *priority);
> +/**
> + * Restore the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_restore(const u8 *priority, u32 num);
>
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
>
> diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> index 48c24f0..38704a1 100644
> --- a/include/sbi_utils/irqchip/plic.h
> +++ b/include/sbi_utils/irqchip/plic.h
> @@ -18,9 +18,10 @@ struct plic_data {
>  };
>
>  /* So far, priorities on all consumers of these functions fit in 8 bits. */
> -void plic_priority_save(const struct plic_data *plic, u8 *priority);
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num);
>
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority);
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +                          u32 num);
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
>                        u32 *enable, u32 *threshold);
> diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> index fe08836..7d76c5b 100644
> --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> @@ -24,18 +24,18 @@ static struct plic_data plic[PLIC_MAX_NR];
>  static struct plic_data *plic_hartid2data[SBI_HARTMASK_MAX_BITS];
>  static int plic_hartid2context[SBI_HARTMASK_MAX_BITS][2];
>
> -void fdt_plic_priority_save(u8 *priority)
> +void fdt_plic_priority_save(u8 *priority, u32 num)
>  {
>         struct plic_data *plic = plic_hartid2data[current_hartid()];
>
> -       plic_priority_save(plic, priority);
> +       plic_priority_save(plic, priority, num);
>  }
>
> -void fdt_plic_priority_restore(const u8 *priority)
> +void fdt_plic_priority_restore(const u8 *priority, u32 num)
>  {
>         struct plic_data *plic = plic_hartid2data[current_hartid()];
>
> -       plic_priority_restore(plic, priority);
> +       plic_priority_restore(plic, priority, num);
>  }
>
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 4df9020..dca5678 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -36,15 +36,22 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
>         writel(val, plic_priority);
>  }
>
> -void plic_priority_save(const struct plic_data *plic, u8 *priority)
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
>  {
> -       for (u32 i = 1; i <= plic->num_src; i++)
> +       if (num > plic->num_src)
> +               num = plic->num_src;
> +
> +       for (u32 i = 1; i <= num; i++)
>                 priority[i] = plic_get_priority(plic, i);
>  }
>
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +                          u32 num)
>  {
> -       for (u32 i = 1; i <= plic->num_src; i++)
> +       if (num > plic->num_src)
> +               num = plic->num_src;
> +
> +       for (u32 i = 1; i <= num; i++)
>                 plic_set_priority(plic, i, priority[i]);
>  }
>
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 18d330d..1f27575 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -79,13 +79,13 @@ static u32 plic_threshold;
>  static void sun20i_d1_plic_save(void)
>  {
>         fdt_plic_context_save(true, plic_sie, &plic_threshold);
> -       fdt_plic_priority_save(plic_priority);
> +       fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>
>  static void sun20i_d1_plic_restore(void)
>  {
>         thead_plic_restore();
> -       fdt_plic_priority_restore(plic_priority);
> +       fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>         fdt_plic_context_restore(true, plic_sie, plic_threshold);
>  }
>
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 5/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:25 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Currently the context save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
>
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

>
> ---
>
> Changes in v3:
> - move the size limit check to plic_context_save/restore
>
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers
>
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h |  5 +++--
>  include/sbi_utils/irqchip/plic.h             |  4 ++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  9 +++++----
>  lib/utils/irqchip/plic.c                     | 14 ++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  5 +++--
>  5 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> index d5b1c60..df645dd 100644
> --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> @@ -23,9 +23,10 @@ void fdt_plic_priority_save(u8 *priority, u32 num);
>   */
>  void fdt_plic_priority_restore(const u8 *priority, u32 num);
>
> -void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
> +void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num);
>
> -void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold);
> +void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
> +                             u32 num);
>
>  void thead_plic_restore(void);
>
> diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> index 38704a1..112a714 100644
> --- a/include/sbi_utils/irqchip/plic.h
> +++ b/include/sbi_utils/irqchip/plic.h
> @@ -24,10 +24,10 @@ void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
>                            u32 num);
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
> -                      u32 *enable, u32 *threshold);
> +                      u32 *enable, u32 *threshold, u32 num);
>
>  void plic_context_restore(const struct plic_data *plic, int context_id,
> -                         const u32 *enable, u32 threshold);
> +                         const u32 *enable, u32 threshold, u32 num);
>
>  int plic_context_init(const struct plic_data *plic, int context_id,
>                       bool enable, u32 threshold);
> diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> index 7d76c5b..c54f45c 100644
> --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> @@ -38,22 +38,23 @@ void fdt_plic_priority_restore(const u8 *priority, u32 num)
>         plic_priority_restore(plic, priority, num);
>  }
>
> -void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> +void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num)
>  {
>         u32 hartid = current_hartid();
>
>         plic_context_save(plic_hartid2data[hartid],
>                           plic_hartid2context[hartid][smode],
> -                         enable, threshold);
> +                         enable, threshold, num);
>  }
>
> -void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold)
> +void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
> +                             u32 num)
>  {
>         u32 hartid = current_hartid();
>
>         plic_context_restore(plic_hartid2data[hartid],
>                              plic_hartid2context[hartid][smode],
> -                            enable, threshold);
> +                            enable, threshold, num);
>  }
>
>  static int irqchip_plic_warm_init(void)
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index b152bb7..8089a0b 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -98,22 +98,28 @@ static void plic_set_ie(const struct plic_data *plic, u32 cntxid,
>  }
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
> -                      u32 *enable, u32 *threshold)
> +                      u32 *enable, u32 *threshold, u32 num)
>  {
>         u32 ie_words = plic->num_src / 32 + 1;
>
> -       for (u32 i = 0; i < ie_words; i++)
> +       if (num > ie_words)
> +               num = ie_words;
> +
> +       for (u32 i = 0; i < num; i++)
>                 enable[i] = plic_get_ie(plic, context_id, i);
>
>         *threshold = plic_get_thresh(plic, context_id);
>  }
>
>  void plic_context_restore(const struct plic_data *plic, int context_id,
> -                         const u32 *enable, u32 threshold)
> +                         const u32 *enable, u32 threshold, u32 num)
>  {
>         u32 ie_words = plic->num_src / 32 + 1;
>
> -       for (u32 i = 0; i < ie_words; i++)
> +       if (num > ie_words)
> +               num = ie_words;
> +
> +       for (u32 i = 0; i < num; i++)
>                 plic_set_ie(plic, context_id, i, enable[i]);
>
>         plic_set_thresh(plic, context_id, threshold);
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 1f27575..1da9e5b 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -78,7 +78,7 @@ static u32 plic_threshold;
>
>  static void sun20i_d1_plic_save(void)
>  {
> -       fdt_plic_context_save(true, plic_sie, &plic_threshold);
> +       fdt_plic_context_save(true, plic_sie, &plic_threshold, PLIC_IE_WORDS);
>         fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>
> @@ -86,7 +86,8 @@ static void sun20i_d1_plic_restore(void)
>  {
>         thead_plic_restore();
>         fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
> -       fdt_plic_context_restore(true, plic_sie, plic_threshold);
> +       fdt_plic_context_restore(true, plic_sie, plic_threshold,
> +                                PLIC_IE_WORDS);
>  }
>
>  /*
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Andreas Schwab]

On Dez 11 2022, Bin Meng wrote:

> I added function parameter descriptions in patch 2 to make it crystal
> clear, that the priority array needs to include interrupt source 0.

It should say that the array size must be one more than the maximum
number of sources.

-- 
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 v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Andreas Schwab]

On Dez 11 2022, Bin Meng wrote:

> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 18d330d..1f27575 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -79,13 +79,13 @@ static u32 plic_threshold;
>  static void sun20i_d1_plic_save(void)
>  {
>  	fdt_plic_context_save(true, plic_sie, &plic_threshold);
> -	fdt_plic_priority_save(plic_priority);
> +	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>  
>  static void sun20i_d1_plic_restore(void)
>  {
>  	thead_plic_restore();
> -	fdt_plic_priority_restore(plic_priority);
> +	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>  	fdt_plic_context_restore(true, plic_sie, plic_threshold);
>  }

That fails to update the size of the plic_priority arraay.

-- 
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 v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Samuel Holland]

On 12/11/22 04:18, Bin Meng wrote:
> On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 11 2022, Bin Meng wrote:
>>
>>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
>>> index 73d7788..4df9020 100644
>>> --- a/lib/utils/irqchip/plic.c
>>> +++ b/lib/utils/irqchip/plic.c
>>> @@ -38,13 +38,13 @@ 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)
>>>  {
>>> -     for (u32 i = 0; i < plic->num_src; i++)
>>> +     for (u32 i = 1; i <= plic->num_src; i++)
>>>               priority[i] = plic_get_priority(plic, i);
>>
>> That needs to adjust the index into the priority array.
> 
> Does that help anything? It just confuses people more.
> 
> I added function parameter descriptions in patch 2 to make it crystal
> clear, that the priority array needs to include interrupt source 0.

To me, it is more confusing that when I ask to save the priority for N
sources, I need to allocate an array with >N elements. And leaving array
element 0 unused wastes memory.

Regards,
Samuel

[PATCH v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Samuel Holland]

On 12/11/22 06:11, Andreas Schwab wrote:
> On Dez 11 2022, Bin Meng wrote:
> 
>> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
>> index 18d330d..1f27575 100644
>> --- a/platform/generic/allwinner/sun20i-d1.c
>> +++ b/platform/generic/allwinner/sun20i-d1.c
>> @@ -79,13 +79,13 @@ static u32 plic_threshold;
>>  static void sun20i_d1_plic_save(void)
>>  {
>>  	fdt_plic_context_save(true, plic_sie, &plic_threshold);
>> -	fdt_plic_priority_save(plic_priority);
>> +	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>>  }
>>  
>>  static void sun20i_d1_plic_restore(void)
>>  {
>>  	thead_plic_restore();
>> -	fdt_plic_priority_restore(plic_priority);
>> +	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>>  	fdt_plic_context_restore(true, plic_sie, plic_threshold);
>>  }
> 
> That fails to update the size of the plic_priority array.

PLIC_SOURCES is 176, and I later corrected riscv,ndev in the DT to 175,
so technically this does not create a bug.

Regards,
Samuel

[PATCH v3 3/5] lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()

[Samuel Holland]

On 12/11/22 00:54, Bin Meng wrote:
> The number of interrupt enable register in words was once correct,
> but was wrongly changed to have an off-by-one error since
> commit 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function").

Indeed, as far as I can tell from my recent reading, I misinterpreted
the binding. I don't like that it excludes the reserved source 0, but I
don't get to make that decision.

> Fixes: 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Reviewed-by: Samuel Holland <samuel@sholland.org>

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()
> 
>  lib/utils/irqchip/plic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index dca5678..568f814 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -127,7 +127,7 @@ int plic_context_init(const struct plic_data *plic, int context_id,
>  	if (!plic || context_id < 0)
>  		return SBI_EINVAL;
>  
> -	ie_words = (plic->num_src + 31) / 32;
> +	ie_words = plic->num_src / 32 + 1;
>  	ie_value = enable ? 0xffffffffU : 0U;
>  
>  	for (u32 i = 0; i < ie_words; i++)

[PATCH v3 4/5] lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers

[Samuel Holland]

On 12/11/22 00:54, Bin Meng wrote:
> plic->num_src holds the number of interrupt sources without interrupt
> source 0 but the interrupt enable register includes a bit for the
> interrupt source 0 in the first word.

On the other hand, it is not really feasible to exclude the first bit
here, so maybe it makes sense to leave patch 1 as it is for consistency.

> Fixes: 415ecf28f7ad ("lib: irqchip/plic: Add context save/restore helpers")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> 
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers
> 
>  lib/utils/irqchip/plic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Samuel Holland <samuel@sholland.org>

[PATCH v3 5/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

[Samuel Holland]

On 12/11/22 00:54, Bin Meng wrote:
> Currently the context save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
> 
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
> 
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> 
> ---
> 
> Changes in v3:
> - move the size limit check to plic_context_save/restore
> 
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers
> 
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h |  5 +++--
>  include/sbi_utils/irqchip/plic.h             |  4 ++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  9 +++++----
>  lib/utils/irqchip/plic.c                     | 14 ++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  5 +++--
>  5 files changed, 23 insertions(+), 14 deletions(-)
> 
[...]
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 1f27575..1da9e5b 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -78,7 +78,7 @@ static u32 plic_threshold;
>  
>  static void sun20i_d1_plic_save(void)
>  {
> -	fdt_plic_context_save(true, plic_sie, &plic_threshold);
> +	fdt_plic_context_save(true, plic_sie, &plic_threshold, PLIC_IE_WORDS);

While the actual value ends up being the same, you should update the
definition of PLIC_IE_WORDS to use the same formula used elsewhere.

Regards,
Samuel

>  	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>  
> @@ -86,7 +86,8 @@ static void sun20i_d1_plic_restore(void)
>  {
>  	thead_plic_restore();
>  	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
> -	fdt_plic_context_restore(true, plic_sie, plic_threshold);
> +	fdt_plic_context_restore(true, plic_sie, plic_threshold,
> +				 PLIC_IE_WORDS);
>  }
>  
>  /*

[PATCH v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Samuel Holland]

On 12/11/22 00:54, Bin Meng wrote:
> Currently the priority save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
> 
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
> 
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> 
> ---
> 
> Changes in v3:
> - fix the size limit check
> - move the size limit check to plic_priority_save/restore
> - add parameter description to fdt_plic_priority_save/restore
> 
> Changes in v2:
> - new patch: libs: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers
> 
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h | 14 ++++++++++++--
>  include/sbi_utils/irqchip/plic.h             |  5 +++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  8 ++++----
>  lib/utils/irqchip/plic.c                     | 15 +++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  4 ++--
>  5 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> index 98d4de5..d5b1c60 100644
> --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> @@ -9,9 +9,19 @@
>  
>  #include <sbi/sbi_types.h>
>  
> -void fdt_plic_priority_save(u8 *priority);
> +/**
> + * Save the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_save(u8 *priority, u32 num);
>  
> -void fdt_plic_priority_restore(const u8 *priority);
> +/**
> + * Restore the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_restore(const u8 *priority, u32 num);
>  
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
>  
> diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> index 48c24f0..38704a1 100644
> --- a/include/sbi_utils/irqchip/plic.h
> +++ b/include/sbi_utils/irqchip/plic.h
> @@ -18,9 +18,10 @@ struct plic_data {
>  };
>  
>  /* So far, priorities on all consumers of these functions fit in 8 bits. */
> -void plic_priority_save(const struct plic_data *plic, u8 *priority);
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num);
>  
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority);
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +			   u32 num);
>  
>  void plic_context_save(const struct plic_data *plic, int context_id,
>  		       u32 *enable, u32 *threshold);
> diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> index fe08836..7d76c5b 100644
> --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> @@ -24,18 +24,18 @@ static struct plic_data plic[PLIC_MAX_NR];
>  static struct plic_data *plic_hartid2data[SBI_HARTMASK_MAX_BITS];
>  static int plic_hartid2context[SBI_HARTMASK_MAX_BITS][2];
>  
> -void fdt_plic_priority_save(u8 *priority)
> +void fdt_plic_priority_save(u8 *priority, u32 num)
>  {
>  	struct plic_data *plic = plic_hartid2data[current_hartid()];
>  
> -	plic_priority_save(plic, priority);
> +	plic_priority_save(plic, priority, num);
>  }
>  
> -void fdt_plic_priority_restore(const u8 *priority)
> +void fdt_plic_priority_restore(const u8 *priority, u32 num)
>  {
>  	struct plic_data *plic = plic_hartid2data[current_hartid()];
>  
> -	plic_priority_restore(plic, priority);
> +	plic_priority_restore(plic, priority, num);
>  }
>  
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 4df9020..dca5678 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -36,15 +36,22 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
>  	writel(val, plic_priority);
>  }
>  
> -void plic_priority_save(const struct plic_data *plic, u8 *priority)
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
>  {
> -	for (u32 i = 1; i <= plic->num_src; i++)
> +	if (num > plic->num_src)
> +		num = plic->num_src;

This check is not really necessary. If the caller tries to save/restore
more sources then actually exist, then the extra register writes will
just be ignored. We already access extra bits by doing word accesses.

For bounds checking, the only things that need to match are the array
size and the loop. With your change to the loop condition, plic->num_src
is not relevant at all anymore.

Regards,
Samuel

> +
> +	for (u32 i = 1; i <= num; i++)
>  		priority[i] = plic_get_priority(plic, i);
>  }
>  
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +			   u32 num)
>  {
> -	for (u32 i = 1; i <= plic->num_src; i++)
> +	if (num > plic->num_src)
> +		num = plic->num_src;
> +
> +	for (u32 i = 1; i <= num; i++)
>  		plic_set_priority(plic, i, priority[i]);
>  }
>  
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 18d330d..1f27575 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -79,13 +79,13 @@ static u32 plic_threshold;
>  static void sun20i_d1_plic_save(void)
>  {
>  	fdt_plic_context_save(true, plic_sie, &plic_threshold);
> -	fdt_plic_priority_save(plic_priority);
> +	fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>  
>  static void sun20i_d1_plic_restore(void)
>  {
>  	thead_plic_restore();
> -	fdt_plic_priority_restore(plic_priority);
> +	fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>  	fdt_plic_context_restore(true, plic_sie, plic_threshold);
>  }
>  

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Anup Patel]

On Mon, Dec 12, 2022 at 12:10 PM Samuel Holland <samuel@sholland.org> wrote:
>
> On 12/11/22 04:18, Bin Meng wrote:
> > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >> On Dez 11 2022, Bin Meng wrote:
> >>
> >>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> >>> index 73d7788..4df9020 100644
> >>> --- a/lib/utils/irqchip/plic.c
> >>> +++ b/lib/utils/irqchip/plic.c
> >>> @@ -38,13 +38,13 @@ 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)
> >>>  {
> >>> -     for (u32 i = 0; i < plic->num_src; i++)
> >>> +     for (u32 i = 1; i <= plic->num_src; i++)
> >>>               priority[i] = plic_get_priority(plic, i);
> >>
> >> That needs to adjust the index into the priority array.
> >
> > Does that help anything? It just confuses people more.
> >
> > I added function parameter descriptions in patch 2 to make it crystal
> > clear, that the priority array needs to include interrupt source 0.
>
> To me, it is more confusing that when I ask to save the priority for N
> sources, I need to allocate an array with >N elements. And leaving array
> element 0 unused wastes memory.

This can be a separate improvement patch.

Regards,
Anup

>
> Regards,
> Samuel
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Anup Patel]

On Mon, Dec 12, 2022 at 12:34 PM Samuel Holland <samuel@sholland.org> wrote:
>
> On 12/11/22 00:54, Bin Meng wrote:
> > Currently the priority save/restore helpers writes/reads the provided
> > array using an index whose maximum value is determined by PLIC, which
> > potentially may disagree with the caller to these helpers.
> >
> > Add a parameter to ask the caller to provide the size limit of the
> > array to ensure no out-of-bound access happens.
> >
> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> >
> > ---
> >
> > Changes in v3:
> > - fix the size limit check
> > - move the size limit check to plic_priority_save/restore
> > - add parameter description to fdt_plic_priority_save/restore
> >
> > Changes in v2:
> > - new patch: libs: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers
> >
> >  include/sbi_utils/irqchip/fdt_irqchip_plic.h | 14 ++++++++++++--
> >  include/sbi_utils/irqchip/plic.h             |  5 +++--
> >  lib/utils/irqchip/fdt_irqchip_plic.c         |  8 ++++----
> >  lib/utils/irqchip/plic.c                     | 15 +++++++++++----
> >  platform/generic/allwinner/sun20i-d1.c       |  4 ++--
> >  5 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> > index 98d4de5..d5b1c60 100644
> > --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> > +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> > @@ -9,9 +9,19 @@
> >
> >  #include <sbi/sbi_types.h>
> >
> > -void fdt_plic_priority_save(u8 *priority);
> > +/**
> > + * Save the PLIC priority state
> > + * @param priority pointer to the memory region for the saved priority
> > + * @param num size of the memory region including interrupt source 0
> > + */
> > +void fdt_plic_priority_save(u8 *priority, u32 num);
> >
> > -void fdt_plic_priority_restore(const u8 *priority);
> > +/**
> > + * Restore the PLIC priority state
> > + * @param priority pointer to the memory region for the saved priority
> > + * @param num size of the memory region including interrupt source 0
> > + */
> > +void fdt_plic_priority_restore(const u8 *priority, u32 num);
> >
> >  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
> >
> > diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> > index 48c24f0..38704a1 100644
> > --- a/include/sbi_utils/irqchip/plic.h
> > +++ b/include/sbi_utils/irqchip/plic.h
> > @@ -18,9 +18,10 @@ struct plic_data {
> >  };
> >
> >  /* So far, priorities on all consumers of these functions fit in 8 bits. */
> > -void plic_priority_save(const struct plic_data *plic, u8 *priority);
> > +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num);
> >
> > -void plic_priority_restore(const struct plic_data *plic, const u8 *priority);
> > +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> > +                        u32 num);
> >
> >  void plic_context_save(const struct plic_data *plic, int context_id,
> >                      u32 *enable, u32 *threshold);
> > diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> > index fe08836..7d76c5b 100644
> > --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> > +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> > @@ -24,18 +24,18 @@ static struct plic_data plic[PLIC_MAX_NR];
> >  static struct plic_data *plic_hartid2data[SBI_HARTMASK_MAX_BITS];
> >  static int plic_hartid2context[SBI_HARTMASK_MAX_BITS][2];
> >
> > -void fdt_plic_priority_save(u8 *priority)
> > +void fdt_plic_priority_save(u8 *priority, u32 num)
> >  {
> >       struct plic_data *plic = plic_hartid2data[current_hartid()];
> >
> > -     plic_priority_save(plic, priority);
> > +     plic_priority_save(plic, priority, num);
> >  }
> >
> > -void fdt_plic_priority_restore(const u8 *priority)
> > +void fdt_plic_priority_restore(const u8 *priority, u32 num)
> >  {
> >       struct plic_data *plic = plic_hartid2data[current_hartid()];
> >
> > -     plic_priority_restore(plic, priority);
> > +     plic_priority_restore(plic, priority, num);
> >  }
> >
> >  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> > index 4df9020..dca5678 100644
> > --- a/lib/utils/irqchip/plic.c
> > +++ b/lib/utils/irqchip/plic.c
> > @@ -36,15 +36,22 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
> >       writel(val, plic_priority);
> >  }
> >
> > -void plic_priority_save(const struct plic_data *plic, u8 *priority)
> > +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
> >  {
> > -     for (u32 i = 1; i <= plic->num_src; i++)
> > +     if (num > plic->num_src)
> > +             num = plic->num_src;
>
> This check is not really necessary. If the caller tries to save/restore
> more sources then actually exist, then the extra register writes will
> just be ignored. We already access extra bits by doing word accesses.
>
> For bounds checking, the only things that need to match are the array
> size and the loop. With your change to the loop condition, plic->num_src
> is not relevant at all anymore.

Okay, I will drop the bound checks at the time of merging this patch
since the motivation of this patch is mainly to avoid out-of-bound
access to the array passed by the caller.

If required, let's have a separate patch for this.

Regards,
Anup

>
> Regards,
> Samuel
>
> > +
> > +     for (u32 i = 1; i <= num; i++)
> >               priority[i] = plic_get_priority(plic, i);
> >  }
> >
> > -void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
> > +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> > +                        u32 num)
> >  {
> > -     for (u32 i = 1; i <= plic->num_src; i++)
> > +     if (num > plic->num_src)
> > +             num = plic->num_src;
> > +
> > +     for (u32 i = 1; i <= num; i++)
> >               plic_set_priority(plic, i, priority[i]);
> >  }
> >
> > diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> > index 18d330d..1f27575 100644
> > --- a/platform/generic/allwinner/sun20i-d1.c
> > +++ b/platform/generic/allwinner/sun20i-d1.c
> > @@ -79,13 +79,13 @@ static u32 plic_threshold;
> >  static void sun20i_d1_plic_save(void)
> >  {
> >       fdt_plic_context_save(true, plic_sie, &plic_threshold);
> > -     fdt_plic_priority_save(plic_priority);
> > +     fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
> >  }
> >
> >  static void sun20i_d1_plic_restore(void)
> >  {
> >       thead_plic_restore();
> > -     fdt_plic_priority_restore(plic_priority);
> > +     fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
> >       fdt_plic_context_restore(true, plic_sie, plic_threshold);
> >  }
> >
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 5/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

[Anup Patel]

On Mon, Dec 12, 2022 at 12:30 PM Samuel Holland <samuel@sholland.org> wrote:
>
> On 12/11/22 00:54, Bin Meng wrote:
> > Currently the context save/restore helpers writes/reads the provided
> > array using an index whose maximum value is determined by PLIC, which
> > potentially may disagree with the caller to these helpers.
> >
> > Add a parameter to ask the caller to provide the size limit of the
> > array to ensure no out-of-bound access happens.
> >
> > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> >
> > ---
> >
> > Changes in v3:
> > - move the size limit check to plic_context_save/restore
> >
> > Changes in v2:
> > - new patch: lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers
> >
> >  include/sbi_utils/irqchip/fdt_irqchip_plic.h |  5 +++--
> >  include/sbi_utils/irqchip/plic.h             |  4 ++--
> >  lib/utils/irqchip/fdt_irqchip_plic.c         |  9 +++++----
> >  lib/utils/irqchip/plic.c                     | 14 ++++++++++----
> >  platform/generic/allwinner/sun20i-d1.c       |  5 +++--
> >  5 files changed, 23 insertions(+), 14 deletions(-)
> >
> [...]
> > diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> > index 1f27575..1da9e5b 100644
> > --- a/platform/generic/allwinner/sun20i-d1.c
> > +++ b/platform/generic/allwinner/sun20i-d1.c
> > @@ -78,7 +78,7 @@ static u32 plic_threshold;
> >
> >  static void sun20i_d1_plic_save(void)
> >  {
> > -     fdt_plic_context_save(true, plic_sie, &plic_threshold);
> > +     fdt_plic_context_save(true, plic_sie, &plic_threshold, PLIC_IE_WORDS);
>
> While the actual value ends up being the same, you should update the
> definition of PLIC_IE_WORDS to use the same formula used elsewhere.

Let's have a separate patch for this.

Regards,
Anup

>
> Regards,
> Samuel
>
> >       fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
> >  }
> >
> > @@ -86,7 +86,8 @@ static void sun20i_d1_plic_restore(void)
> >  {
> >       thead_plic_restore();
> >       fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
> > -     fdt_plic_context_restore(true, plic_sie, plic_threshold);
> > +     fdt_plic_context_restore(true, plic_sie, plic_threshold,
> > +                              PLIC_IE_WORDS);
> >  }
> >
> >  /*
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:24 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Interrupt source 0 is reserved. Hence the irq should start from 1.
>
> Fixes: 2b79b694a805 ("lib: irqchip/plic: Add priority save/restore helpers")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>
> (no changes since v1)
>
>  lib/utils/irqchip/plic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 73d7788..4df9020 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -38,13 +38,13 @@ 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)
>  {
> -       for (u32 i = 0; i < plic->num_src; i++)
> +       for (u32 i = 1; i <= plic->num_src; i++)
>                 priority[i] = plic_get_priority(plic, i);
>  }
>
>  void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
>  {
> -       for (u32 i = 0; i < plic->num_src; i++)
> +       for (u32 i = 1; i <= plic->num_src; i++)
>                 plic_set_priority(plic, i, priority[i]);
>  }
>
> --
> 2.34.1
>

[PATCH v3 2/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:25 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Currently the priority save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
>
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

>
> ---
>
> Changes in v3:
> - fix the size limit check
> - move the size limit check to plic_priority_save/restore
> - add parameter description to fdt_plic_priority_save/restore
>
> Changes in v2:
> - new patch: libs: utils/irqchip: plic: Ensure no out-of-bound access in priority save/restore helpers
>
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h | 14 ++++++++++++--
>  include/sbi_utils/irqchip/plic.h             |  5 +++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  8 ++++----
>  lib/utils/irqchip/plic.c                     | 15 +++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  4 ++--
>  5 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> index 98d4de5..d5b1c60 100644
> --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> @@ -9,9 +9,19 @@
>
>  #include <sbi/sbi_types.h>
>
> -void fdt_plic_priority_save(u8 *priority);
> +/**
> + * Save the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_save(u8 *priority, u32 num);
>
> -void fdt_plic_priority_restore(const u8 *priority);
> +/**
> + * Restore the PLIC priority state
> + * @param priority pointer to the memory region for the saved priority
> + * @param num size of the memory region including interrupt source 0
> + */
> +void fdt_plic_priority_restore(const u8 *priority, u32 num);
>
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
>
> diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> index 48c24f0..38704a1 100644
> --- a/include/sbi_utils/irqchip/plic.h
> +++ b/include/sbi_utils/irqchip/plic.h
> @@ -18,9 +18,10 @@ struct plic_data {
>  };
>
>  /* So far, priorities on all consumers of these functions fit in 8 bits. */
> -void plic_priority_save(const struct plic_data *plic, u8 *priority);
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num);
>
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority);
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +                          u32 num);
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
>                        u32 *enable, u32 *threshold);
> diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> index fe08836..7d76c5b 100644
> --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> @@ -24,18 +24,18 @@ static struct plic_data plic[PLIC_MAX_NR];
>  static struct plic_data *plic_hartid2data[SBI_HARTMASK_MAX_BITS];
>  static int plic_hartid2context[SBI_HARTMASK_MAX_BITS][2];
>
> -void fdt_plic_priority_save(u8 *priority)
> +void fdt_plic_priority_save(u8 *priority, u32 num)
>  {
>         struct plic_data *plic = plic_hartid2data[current_hartid()];
>
> -       plic_priority_save(plic, priority);
> +       plic_priority_save(plic, priority, num);
>  }
>
> -void fdt_plic_priority_restore(const u8 *priority)
> +void fdt_plic_priority_restore(const u8 *priority, u32 num)
>  {
>         struct plic_data *plic = plic_hartid2data[current_hartid()];
>
> -       plic_priority_restore(plic, priority);
> +       plic_priority_restore(plic, priority, num);
>  }
>
>  void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 4df9020..dca5678 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -36,15 +36,22 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val)
>         writel(val, plic_priority);
>  }
>
> -void plic_priority_save(const struct plic_data *plic, u8 *priority)
> +void plic_priority_save(const struct plic_data *plic, u8 *priority, u32 num)
>  {
> -       for (u32 i = 1; i <= plic->num_src; i++)
> +       if (num > plic->num_src)
> +               num = plic->num_src;
> +
> +       for (u32 i = 1; i <= num; i++)
>                 priority[i] = plic_get_priority(plic, i);
>  }
>
> -void plic_priority_restore(const struct plic_data *plic, const u8 *priority)
> +void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
> +                          u32 num)
>  {
> -       for (u32 i = 1; i <= plic->num_src; i++)
> +       if (num > plic->num_src)
> +               num = plic->num_src;
> +
> +       for (u32 i = 1; i <= num; i++)
>                 plic_set_priority(plic, i, priority[i]);
>  }
>
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 18d330d..1f27575 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -79,13 +79,13 @@ static u32 plic_threshold;
>  static void sun20i_d1_plic_save(void)
>  {
>         fdt_plic_context_save(true, plic_sie, &plic_threshold);
> -       fdt_plic_priority_save(plic_priority);
> +       fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>
>  static void sun20i_d1_plic_restore(void)
>  {
>         thead_plic_restore();
> -       fdt_plic_priority_restore(plic_priority);
> +       fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
>         fdt_plic_context_restore(true, plic_sie, plic_threshold);
>  }
>
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 3/5] lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()

[Anup Patel]

On Sun, Dec 11, 2022 at 12:24 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> The number of interrupt enable register in words was once correct,
> but was wrongly changed to have an off-by-one error since
> commit 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function").
>
> Fixes: 8c362e7d065e ("lib: irqchip/plic: Factor out a context init function")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Fix the off-by-one error in plic_context_init()
>
>  lib/utils/irqchip/plic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index dca5678..568f814 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -127,7 +127,7 @@ int plic_context_init(const struct plic_data *plic, int context_id,
>         if (!plic || context_id < 0)
>                 return SBI_EINVAL;
>
> -       ie_words = (plic->num_src + 31) / 32;
> +       ie_words = plic->num_src / 32 + 1;
>         ie_value = enable ? 0xffffffffU : 0U;
>
>         for (u32 i = 0; i < ie_words; i++)
> --
> 2.34.1
>

[PATCH v3 4/5] lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:24 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> plic->num_src holds the number of interrupt sources without interrupt
> source 0 but the interrupt enable register includes a bit for the
> interrupt source 0 in the first word.
>
> Fixes: 415ecf28f7ad ("lib: irqchip/plic: Add context save/restore helpers")
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Fix the off-by-one error in context save/restore helpers
>
>  lib/utils/irqchip/plic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index 568f814..b152bb7 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -100,7 +100,7 @@ static void plic_set_ie(const struct plic_data *plic, u32 cntxid,
>  void plic_context_save(const struct plic_data *plic, int context_id,
>                        u32 *enable, u32 *threshold)
>  {
> -       u32 ie_words = (plic->num_src + 31) / 32;
> +       u32 ie_words = plic->num_src / 32 + 1;
>
>         for (u32 i = 0; i < ie_words; i++)
>                 enable[i] = plic_get_ie(plic, context_id, i);
> @@ -111,7 +111,7 @@ void plic_context_save(const struct plic_data *plic, int context_id,
>  void plic_context_restore(const struct plic_data *plic, int context_id,
>                           const u32 *enable, u32 threshold)
>  {
> -       u32 ie_words = (plic->num_src + 31) / 32;
> +       u32 ie_words = plic->num_src / 32 + 1;
>
>         for (u32 i = 0; i < ie_words; i++)
>                 plic_set_ie(plic, context_id, i, enable[i]);
> --
> 2.34.1
>

[PATCH v3 5/5] lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers

[Anup Patel]

On Sun, Dec 11, 2022 at 12:25 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Currently the context save/restore helpers writes/reads the provided
> array using an index whose maximum value is determined by PLIC, which
> potentially may disagree with the caller to these helpers.
>
> Add a parameter to ask the caller to provide the size limit of the
> array to ensure no out-of-bound access happens.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

>
> ---
>
> Changes in v3:
> - move the size limit check to plic_context_save/restore
>
> Changes in v2:
> - new patch: lib: utils/irqchip: plic: Ensure no out-of-bound access in context save/restore helpers
>
>  include/sbi_utils/irqchip/fdt_irqchip_plic.h |  5 +++--
>  include/sbi_utils/irqchip/plic.h             |  4 ++--
>  lib/utils/irqchip/fdt_irqchip_plic.c         |  9 +++++----
>  lib/utils/irqchip/plic.c                     | 14 ++++++++++----
>  platform/generic/allwinner/sun20i-d1.c       |  5 +++--
>  5 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/include/sbi_utils/irqchip/fdt_irqchip_plic.h b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> index d5b1c60..df645dd 100644
> --- a/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> +++ b/include/sbi_utils/irqchip/fdt_irqchip_plic.h
> @@ -23,9 +23,10 @@ void fdt_plic_priority_save(u8 *priority, u32 num);
>   */
>  void fdt_plic_priority_restore(const u8 *priority, u32 num);
>
> -void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold);
> +void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num);
>
> -void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold);
> +void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
> +                             u32 num);
>
>  void thead_plic_restore(void);
>
> diff --git a/include/sbi_utils/irqchip/plic.h b/include/sbi_utils/irqchip/plic.h
> index 38704a1..112a714 100644
> --- a/include/sbi_utils/irqchip/plic.h
> +++ b/include/sbi_utils/irqchip/plic.h
> @@ -24,10 +24,10 @@ void plic_priority_restore(const struct plic_data *plic, const u8 *priority,
>                            u32 num);
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
> -                      u32 *enable, u32 *threshold);
> +                      u32 *enable, u32 *threshold, u32 num);
>
>  void plic_context_restore(const struct plic_data *plic, int context_id,
> -                         const u32 *enable, u32 threshold);
> +                         const u32 *enable, u32 threshold, u32 num);
>
>  int plic_context_init(const struct plic_data *plic, int context_id,
>                       bool enable, u32 threshold);
> diff --git a/lib/utils/irqchip/fdt_irqchip_plic.c b/lib/utils/irqchip/fdt_irqchip_plic.c
> index 7d76c5b..c54f45c 100644
> --- a/lib/utils/irqchip/fdt_irqchip_plic.c
> +++ b/lib/utils/irqchip/fdt_irqchip_plic.c
> @@ -38,22 +38,23 @@ void fdt_plic_priority_restore(const u8 *priority, u32 num)
>         plic_priority_restore(plic, priority, num);
>  }
>
> -void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold)
> +void fdt_plic_context_save(bool smode, u32 *enable, u32 *threshold, u32 num)
>  {
>         u32 hartid = current_hartid();
>
>         plic_context_save(plic_hartid2data[hartid],
>                           plic_hartid2context[hartid][smode],
> -                         enable, threshold);
> +                         enable, threshold, num);
>  }
>
> -void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold)
> +void fdt_plic_context_restore(bool smode, const u32 *enable, u32 threshold,
> +                             u32 num)
>  {
>         u32 hartid = current_hartid();
>
>         plic_context_restore(plic_hartid2data[hartid],
>                              plic_hartid2context[hartid][smode],
> -                            enable, threshold);
> +                            enable, threshold, num);
>  }
>
>  static int irqchip_plic_warm_init(void)
> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
> index b152bb7..8089a0b 100644
> --- a/lib/utils/irqchip/plic.c
> +++ b/lib/utils/irqchip/plic.c
> @@ -98,22 +98,28 @@ static void plic_set_ie(const struct plic_data *plic, u32 cntxid,
>  }
>
>  void plic_context_save(const struct plic_data *plic, int context_id,
> -                      u32 *enable, u32 *threshold)
> +                      u32 *enable, u32 *threshold, u32 num)
>  {
>         u32 ie_words = plic->num_src / 32 + 1;
>
> -       for (u32 i = 0; i < ie_words; i++)
> +       if (num > ie_words)
> +               num = ie_words;
> +
> +       for (u32 i = 0; i < num; i++)
>                 enable[i] = plic_get_ie(plic, context_id, i);
>
>         *threshold = plic_get_thresh(plic, context_id);
>  }
>
>  void plic_context_restore(const struct plic_data *plic, int context_id,
> -                         const u32 *enable, u32 threshold)
> +                         const u32 *enable, u32 threshold, u32 num)
>  {
>         u32 ie_words = plic->num_src / 32 + 1;
>
> -       for (u32 i = 0; i < ie_words; i++)
> +       if (num > ie_words)
> +               num = ie_words;
> +
> +       for (u32 i = 0; i < num; i++)
>                 plic_set_ie(plic, context_id, i, enable[i]);
>
>         plic_set_thresh(plic, context_id, threshold);
> diff --git a/platform/generic/allwinner/sun20i-d1.c b/platform/generic/allwinner/sun20i-d1.c
> index 1f27575..1da9e5b 100644
> --- a/platform/generic/allwinner/sun20i-d1.c
> +++ b/platform/generic/allwinner/sun20i-d1.c
> @@ -78,7 +78,7 @@ static u32 plic_threshold;
>
>  static void sun20i_d1_plic_save(void)
>  {
> -       fdt_plic_context_save(true, plic_sie, &plic_threshold);
> +       fdt_plic_context_save(true, plic_sie, &plic_threshold, PLIC_IE_WORDS);
>         fdt_plic_priority_save(plic_priority, PLIC_SOURCES);
>  }
>
> @@ -86,7 +86,8 @@ static void sun20i_d1_plic_restore(void)
>  {
>         thead_plic_restore();
>         fdt_plic_priority_restore(plic_priority, PLIC_SOURCES);
> -       fdt_plic_context_restore(true, plic_sie, plic_threshold);
> +       fdt_plic_context_restore(true, plic_sie, plic_threshold,
> +                                PLIC_IE_WORDS);
>  }
>
>  /*
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3 1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers

[Andreas Schwab]

On Dez 17 2022, Anup Patel wrote:

> On Mon, Dec 12, 2022 at 12:10 PM Samuel Holland <samuel@sholland.org> wrote:
>>
>> On 12/11/22 04:18, Bin Meng wrote:
>> > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>> >>
>> >> On Dez 11 2022, Bin Meng wrote:
>> >>
>> >>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c
>> >>> index 73d7788..4df9020 100644
>> >>> --- a/lib/utils/irqchip/plic.c
>> >>> +++ b/lib/utils/irqchip/plic.c
>> >>> @@ -38,13 +38,13 @@ 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)
>> >>>  {
>> >>> -     for (u32 i = 0; i < plic->num_src; i++)
>> >>> +     for (u32 i = 1; i <= plic->num_src; i++)
>> >>>               priority[i] = plic_get_priority(plic, i);
>> >>
>> >> That needs to adjust the index into the priority array.
>> >
>> > Does that help anything? It just confuses people more.
>> >
>> > I added function parameter descriptions in patch 2 to make it crystal
>> > clear, that the priority array needs to include interrupt source 0.
>>
>> To me, it is more confusing that when I ask to save the priority for N
>> sources, I need to allocate an array with >N elements. And leaving array
>> element 0 unused wastes memory.
>
> This can be a separate improvement patch.

It is a bug fix, not just an improvement.

-- 
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."