Re: [PATCH v3] KVM: arm64: Add memory length checks and remove inline in do_ffa_mem_xfer

[Oliver Upton]

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