* [PATCH] platform: generic: andes: validate PMAADDR decoding
@ 2026-08-27 11:54 Pengpeng Hou
2026-08-31 2:59 ` Ben Zong-You Xie
0 siblings, 1 reply; 2+ messages in thread
From: Pengpeng Hou @ 2026-08-27 11:54 UTC (permalink / raw)
To: opensbi; +Cc: Anup Patel, Pengpeng Hou, Lad Prabhakar
decode_pmaaddrx() reconstructs a NAPOT region with signed int shifts. The
QiLai platform configures 2^35 and 2^37 PCIe regions, so decoding the first
enabled region while checking the next one reaches shifts by 32 and 35.
Those shifts are undefined even on RV64.
The all-ones PMAADDR value also violates the sbi_ffz() precondition, and a
decoded base or end may not fit in unsigned long.
Use unsigned shifts and make the decoder reject encodings whose size, base,
or inclusive end cannot be represented. Treat an invalid enabled entry as
overlapping when adding a region, and fail rather than skipping it when
freeing a region.
Fixes: aa56084c4dfb ("platform: generic: andes: add a new Andes SBI call to set up a PMA entry")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Base-commit: 4e79fd7de59f1b2899092c1a84ce68c8ebc68f93
platform/generic/andes/andes_pma.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/platform/generic/andes/andes_pma.c b/platform/generic/andes/andes_pma.c
index ba9a35b..cdc06ac 100644
--- a/platform/generic/andes/andes_pma.c
+++ b/platform/generic/andes/andes_pma.c
@@ -69,10 +69,10 @@ static void set_pmaxcfg(int entry_id, char flags)
csr_write_num(pmacfg_addr, pmacfg_val);
}
-static void decode_pmaaddrx(int entry_id, unsigned long *start,
+static bool decode_pmaaddrx(int entry_id, unsigned long *start,
unsigned long *size)
{
- unsigned long pmaaddr;
+ unsigned long addr, pmaaddr;
int k;
/**
@@ -81,9 +81,23 @@ static void decode_pmaaddrx(int entry_id, unsigned long *start,
* start = 4 * ($pmaaddr - (size / 8) + 1)
*/
pmaaddr = csr_read_num(CSR_PMAADDR0 + entry_id);
+ if (pmaaddr == ~0UL)
+ return false;
+
k = sbi_ffz(pmaaddr);
- *size = 1 << (k + 3);
- *start = (pmaaddr - (1 << k) + 1) << 2;
+ if (k >= __riscv_xlen - 3)
+ return false;
+
+ addr = pmaaddr - (1UL << k) + 1;
+ if (addr > (~0UL >> 2))
+ return false;
+
+ *size = 1UL << (k + 3);
+ *start = addr << 2;
+ if (*start > ~0UL - (*size - 1))
+ return false;
+
+ return true;
}
static bool has_pma_region_overlap(unsigned long start, unsigned long size)
@@ -97,7 +111,8 @@ static bool has_pma_region_overlap(unsigned long start, unsigned long size)
if (is_pma_entry_disable(pmaxcfg))
continue;
- decode_pmaaddrx(i, &_start, &_size);
+ if (!decode_pmaaddrx(i, &_start, &_size))
+ return true;
_end = _start + _size - 1;
if (MAX(start, _start) <= MIN(end, _end)) {
@@ -352,7 +367,8 @@ int andes_sbi_free_pma(unsigned long pa)
if (is_pma_entry_disable(pmaxcfg))
continue;
- decode_pmaaddrx(i, &start, &size);
+ if (!decode_pmaaddrx(i, &start, &size))
+ return SBI_ERR_FAILED;
if (start != pa)
continue;
--
2.50.1 (Apple Git-155)
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] platform: generic: andes: validate PMAADDR decoding
2026-08-27 11:54 [PATCH] platform: generic: andes: validate PMAADDR decoding Pengpeng Hou
@ 2026-08-31 2:59 ` Ben Zong-You Xie
0 siblings, 0 replies; 2+ messages in thread
From: Ben Zong-You Xie @ 2026-08-31 2:59 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: opensbi, Lad Prabhakar, Anup Patel
On Thu, Aug 27, 2026 at 07:54:35PM +0800, Pengpeng Hou wrote:
> decode_pmaaddrx() reconstructs a NAPOT region with signed int shifts. The
> QiLai platform configures 2^35 and 2^37 PCIe regions, so decoding the first
> enabled region while checking the next one reaches shifts by 32 and 35.
> Those shifts are undefined even on RV64.
>
> The all-ones PMAADDR value also violates the sbi_ffz() precondition, and a
> decoded base or end may not fit in unsigned long.
>
> Use unsigned shifts and make the decoder reject encodings whose size, base,
> or inclusive end cannot be represented. Treat an invalid enabled entry as
> overlapping when adding a region, and fail rather than skipping it when
> freeing a region.
>
> Fixes: aa56084c4dfb ("platform: generic: andes: add a new Andes SBI call to set up a PMA entry")
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Base-commit: 4e79fd7de59f1b2899092c1a84ce68c8ebc68f93
>
> platform/generic/andes/andes_pma.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/platform/generic/andes/andes_pma.c b/platform/generic/andes/andes_pma.c
> index ba9a35b..cdc06ac 100644
> --- a/platform/generic/andes/andes_pma.c
> +++ b/platform/generic/andes/andes_pma.c
> @@ -69,10 +69,10 @@ static void set_pmaxcfg(int entry_id, char flags)
> csr_write_num(pmacfg_addr, pmacfg_val);
> }
>
> -static void decode_pmaaddrx(int entry_id, unsigned long *start,
> +static bool decode_pmaaddrx(int entry_id, unsigned long *start,
> unsigned long *size)
> {
> - unsigned long pmaaddr;
> + unsigned long addr, pmaaddr;
> int k;
>
> /**
> @@ -81,9 +81,23 @@ static void decode_pmaaddrx(int entry_id, unsigned long *start,
> * start = 4 * ($pmaaddr - (size / 8) + 1)
> */
> pmaaddr = csr_read_num(CSR_PMAADDR0 + entry_id);
> + if (pmaaddr == ~0UL)
> + return false;
> +
> k = sbi_ffz(pmaaddr);
> - *size = 1 << (k + 3);
> - *start = (pmaaddr - (1 << k) + 1) << 2;
> + if (k >= __riscv_xlen - 3)
> + return false;
> +
> + addr = pmaaddr - (1UL << k) + 1;
> + if (addr > (~0UL >> 2))
> + return false;
> +
> + *size = 1UL << (k + 3);
> + *start = addr << 2;
> + if (*start > ~0UL - (*size - 1))
> + return false;
> +
> + return true;
Hi Pengpeng,
Thanks for catching this.
andes_pma_setup() is the only place that programs a PMA entry, and it
already requires a power-of-two size >= 4096 at a naturally aligned addr,
both of type unsigned long. That guarantees k is exactly log2(size) - 3,
so none of the above checks can ever trigger.
Changing 1 to 1UL is reasonable, but we already sent a patch [1] for this
issue, so I would rather see that one land. No need for a v2.
[1] https://patchwork.ozlabs.org/project/opensbi/patch/20260729092317.2848665-1-randolph@andestech.com/
Thanks,
Ben
> }
>
> static bool has_pma_region_overlap(unsigned long start, unsigned long size)
> @@ -97,7 +111,8 @@ static bool has_pma_region_overlap(unsigned long start, unsigned long size)
> if (is_pma_entry_disable(pmaxcfg))
> continue;
>
> - decode_pmaaddrx(i, &_start, &_size);
> + if (!decode_pmaaddrx(i, &_start, &_size))
> + return true;
> _end = _start + _size - 1;
>
> if (MAX(start, _start) <= MIN(end, _end)) {
> @@ -352,7 +367,8 @@ int andes_sbi_free_pma(unsigned long pa)
> if (is_pma_entry_disable(pmaxcfg))
> continue;
>
> - decode_pmaaddrx(i, &start, &size);
> + if (!decode_pmaaddrx(i, &start, &size))
> + return SBI_ERR_FAILED;
> if (start != pa)
> continue;
>
> --
> 2.50.1 (Apple Git-155)
>
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-08-31 3:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-27 11:54 [PATCH] platform: generic: andes: validate PMAADDR decoding Pengpeng Hou
2026-08-31 2:59 ` Ben Zong-You Xie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox