Re: [PATCH 03/12] xen/arm: ffa: Harden shm page parsing

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Feb 9, 2026 at 4:11 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 9 Feb 2026, at 15:26, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> >
> > Hi Jens,
> >
> >> On 9 Feb 2026, at 10:31, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >>
> >> Hi Bertrand,
> >>
> >> On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
> >> <bertrand.marquis@xxxxxxx> wrote:
> >>>
> >>> get_shm_pages() uses unchecked address arithmetic and does not enforce
> >>> alignment, so malformed descriptors can cause overflow or slip through
> >>> validation. The reclaim path also repeats handle-to-shm-mem conversion
> >>> in multiple places, duplicating error handling.
> >>>
> >>> Harden page parsing and reclaim handling:
> >>> - add ffa_safe_addr_add() and use it to detect address overflows
> >>> - enforce alignment checks in get_shm_pages() and return FF-A errors
> >>> - introduce ffa_secure_reclaim() and use it for MEM_RECLAIM and teardown
> >>> - simplify ffa_mem_share() argument handling and allow max page count
> >>>
> >>> Functional impact: invalid or misaligned memory ranges now fail earlier
> >>> with proper error codes; behavior for valid descriptors is unchanged.
> >>>
> >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >>> ---
> >>> xen/arch/arm/tee/ffa_private.h | 11 +++++++
> >>> xen/arch/arm/tee/ffa_shm.c     | 57 +++++++++++++++++-----------------
> >>> 2 files changed, 40 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/tee/ffa_private.h 
> >>> b/xen/arch/arm/tee/ffa_private.h
> >>> index b625f1c72914..58562d8e733c 100644
> >>> --- a/xen/arch/arm/tee/ffa_private.h
> >>> +++ b/xen/arch/arm/tee/ffa_private.h
> >>> @@ -632,4 +632,15 @@ static inline void ffa_uuid_set(struct ffa_uuid *id, 
> >>> uint32_t val0,
> >>>    id->val[1] = ((uint64_t)val3 << 32U) | val2;
> >>> }
> >>>
> >>> +/*
> >>> + * Common overflow-safe helper to verify that adding a number of pages 
> >>> to an
> >>> + * address will not wrap around.
> >>> + */
> >>> +static inline bool ffa_safe_addr_add(uint64_t addr, uint64_t pages)
> >>> +{
> >>> +    uint64_t off = pages * FFA_PAGE_SIZE;
> >>> +
> >>> +    return (off / FFA_PAGE_SIZE) == pages && addr <= UINT64_MAX - off;
> >>> +}
> >>> +
> >>> #endif /*__FFA_PRIVATE_H__*/
> >>> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> >>> index 90800e44a86a..4c0b45cde6ee 100644
> >>> --- a/xen/arch/arm/tee/ffa_shm.c
> >>> +++ b/xen/arch/arm/tee/ffa_shm.c
> >>> @@ -96,16 +96,14 @@ struct ffa_shm_mem {
> >>>    struct page_info *pages[];
> >>> };
> >>>
> >>> -static int32_t ffa_mem_share(uint32_t tot_len, uint32_t frag_len,
> >>> -                             register_t addr, uint32_t pg_count,
> >>> -                             uint64_t *handle)
> >>> +static int32_t ffa_mem_share(uint32_t tot_len, uint64_t *handle)
> >>> {
> >>>    struct arm_smccc_1_2_regs arg = {
> >>>        .a0 = FFA_MEM_SHARE_64,
> >>>        .a1 = tot_len,
> >>> -        .a2 = frag_len,
> >>> -        .a3 = addr,
> >>> -        .a4 = pg_count,
> >>> +        .a2 = tot_len,
> >>> +        .a3 = 0,
> >>> +        .a4 = 0,
> >>>    };
> >>>    struct arm_smccc_1_2_regs resp;
> >>>
> >>> @@ -131,12 +129,16 @@ static int32_t ffa_mem_share(uint32_t tot_len, 
> >>> uint32_t frag_len,
> >>>    }
> >>> }
> >>>
> >>> -static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
> >>> -                               uint32_t flags)
> >>> +static int32_t ffa_secure_reclaim(struct ffa_shm_mem *shm, uint32_t 
> >>> flags)
> >>
> >> I agree with moving the uint64_to_regpair() call into this function,
> >> but I'm puzzled by the new name. What's secure?
> >
> > This is to distinguish with reclaim for VM to VM sharing in the future as 
> > here
> > reclaim is asked to the secure world.
> >
> > But in fact to be coherent I should also have renamed ffa_mem_share to 
> > ffa_secure_share.
> >
> > Would you be ok with that ?
>
> Looking at this a bit more, we are usually using spmc and not secure.
>
> Would you be ok if I rename both those to:
> ffa_spmc_share
> ffa_spmc_reclaim

Yes, that sounds good.

Cheers,
Jens