Re: [PATCH] KVM: arm64: Add memory length checks before it is xfered

[Sebastian Ene]

On Fri, Sep 06, 2024 at 05:35:39PM +0100, Marc Zyngier wrote:

Hi,

> Hi Snehal,
> 
> On Fri, 06 Sep 2024 10:27:32 +0100,
> Snehal Koukuntla <snehalreddy@xxxxxxxxxx> wrote:
> > 
> > From: Snehal <snehalreddy@xxxxxxxxxx>
> > 
> > Check size during allocation to fix discrepancy in memory reclaim path.
> > Currently only happens during memory reclaim, inconsistent with mem_xfer
> 
> Can you please elaborate? It doesn't seem to fail at allocation time
> here, as everything is pre-allocated. Some context would greatly help,
> as my FFA-foo is as basic as it gets (I did read the spec once and ran
> away screaming).
> 

Right, I think what happens is that we use the fragmentation API to
transfer memory to Trustzone that normally won't fit on the reclaim path
where we use an auxiliary buffer to store the descriptors.

All the descriptors are identified by the same handle and the reclaim
will try to store them into the ffa_desc_buf before nuking the FF-A
annotation from the stage-2.

> >
> > Signed-off-by: Snehal Koukuntla <snehalreddy@xxxxxxxxxx>
> 
> The From: and Signed-off-by: tags do not match. You may want to add a
> [user] section to your .gitconfig with your full name so that this
> issue is sorted once and for all.
> 
> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index e715c157c2c4..e9223cc4f913 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -461,6 +461,11 @@ static __always_inline void do_ffa_mem_xfer(const u64 func_id,
> 
> /facepalm: why do we have this __always_inline here? Nothing to do
> with your patch, but definitely worth understanding why it is
> required.
>

I don't think it is needed, we can drop it. Maybe as part of this patch
?

> >  		goto out_unlock;
> >  	}
> >  
> > +	if (len > ffa_desc_buf.len) {
> > +		ret = FFA_RET_NO_MEMORY;
> > +		goto out_unlock;
> > +	}
> > +
> 
> It took some digging to understand how the various queues are sized,
> and a comment explaining the relation between ffa_desc_buf and the
> other queues would be very welcome.
> 
> I also notice that we have other places (apparently dealing with
> fragments) that do not have such checks. Do they need anything else?
>

I think we don't need that check in other parts.

> >  	buf = hyp_buffers.tx;
> >  	memcpy(buf, host_buffers.tx, fraglen);
> >  
> 
> Finally, this probably deserves a Fixes: tag and a Cc: stable so that
> it can be backported.
> 
> Thanks,
> 
> 	M.
>

Seb

> -- 
> Without deviation from the norm, progress is not possible.