Hi Snehal,
On Mon, Sep 09, 2024 at 06:01:54PM +0000, Snehal Koukuntla wrote:
> When we share memory through FF-A and the description of the buffers
> exceeds the size of the mapped buffer, the fragmentation API is used.
> The fragmentation API allows specifying chunks of descriptors in subsequent
> FF-A fragment calls and no upper limit has been established for this.
> The entire memory region transferred is identified by a handle which can be
> used to reclaim the transferred memory.
> To be able to reclaim the memory, the description of the buffers has to fit
> in the ffa_desc_buf.
> Add a bounds check on the FF-A sharing path to prevent the memory reclaim
> from failing.
>
> Also do_ffa_mem_xfer() does not need __always_inline
>
> Fixes: 634d90cf0ac65 ("KVM: arm64: Handle FFA_MEM_LEND calls from the host")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> Signed-off-by: Snehal Koukuntla <snehalreddy@xxxxxxxxxx>
> ---
Next time around, please include some notes on what's changed between
versions and ideally a link to the last patch. It helps latecomers (i.e.
me) get an idea of what's happening w/ a patch.
> arch/arm64/kvm/hyp/nvhe/ffa.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index e715c157c2c4..637425f63fd1 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -426,7 +426,7 @@ static void do_ffa_mem_frag_tx(struct arm_smccc_res *res,
> return;
> }
>
> -static __always_inline void do_ffa_mem_xfer(const u64 func_id,
> +static void do_ffa_mem_xfer(const u64 func_id,
> struct arm_smccc_res *res,
> struct kvm_cpu_context *ctxt)
> {
> @@ -461,6 +461,11 @@ static __always_inline void do_ffa_mem_xfer(const u64 func_id,
> goto out_unlock;
> }
>
> + if (len > ffa_desc_buf.len) {
> + ret = FFA_RET_NO_MEMORY;
> + goto out_unlock;
> + }
> +
This check doesn't need to happen behind the host_buffers spinlock. Of
course, keeping it behind the lock is benign, but this sort of thing
prompts a reviewer to ask "why?"
Besides that,
Reviewed-by: Oliver Upton <oliver.upton@xxxxxxxxx>
--
Thanks,
Oliver
