Hi Bertrand, On Mon, Feb 9, 2026 at 4:11 PM Bertrand Marquis <[email protected]> wrote: > > Hi Jens, > > > On 9 Feb 2026, at 15:26, Bertrand Marquis <[email protected]> wrote: > > > > Hi Jens, > > > >> On 9 Feb 2026, at 10:31, Jens Wiklander <[email protected]> wrote: > >> > >> Hi Bertrand, > >> > >> On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis > >> <[email protected]> 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 <[email protected]> > >>> --- > >>> 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
