Re: [PATCH] platform: generic: andes: validate PMAADDR decoding

[Ben Zong-You Xie <ben717@andestech.com>]

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