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

[Snehal Koukuntla]

Hi,

Thanks Sebastian for replying to Marc's comments.

> > > --- 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
> ?

I will send an updated patch removing the inline then.

> > 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.

Thanks, I will update my git config to fix the inconsistency.

Thanks,
Snehal


On Mon, Sep 9, 2024 at 8:16 AM Sebastian Ene <sebastianene@xxxxxxxxxx> wrote:
>
> 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.