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

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

[Snehal Koukuntla]

From: Snehal <snehalreddy@google.com>

Check size during allocation to fix discrepancy in memory reclaim path.
Currently only happens during memory reclaim, inconsistent with mem_xfer

Signed-off-by: Snehal Koukuntla <snehalreddy@google.com>
---
 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,
 		goto out_unlock;
 	}
 
+	if (len > ffa_desc_buf.len) {
+		ret = FFA_RET_NO_MEMORY;
+		goto out_unlock;
+	}
+
 	buf = hyp_buffers.tx;
 	memcpy(buf, host_buffers.tx, fraglen);
 
-- 
2.46.0.469.g59c65b2a67-goog

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

[Marc Zyngier]

Hi Snehal,

On Fri, 06 Sep 2024 10:27:32 +0100,
Snehal Koukuntla <snehalreddy@google.com> wrote:
> 
> From: Snehal <snehalreddy@google.com>
> 
> 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).

>
> Signed-off-by: Snehal Koukuntla <snehalreddy@google.com>

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.

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

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

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

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@google.com> wrote:
> > 
> > From: Snehal <snehalreddy@google.com>
> > 
> > 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@google.com>
> 
> 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.

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@google.com> 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@google.com> wrote:
> > >
> > > From: Snehal <snehalreddy@google.com>
> > >
> > > 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@google.com>
> >
> > 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.