Re: [PATCH] KVM: arm64: Validate the FF-A memory access descriptor placement

[Sebastian Ene <sebastianene@google.com>]

On Wed, Apr 22, 2026 at 08:29:06PM +0100, Sudeep Holla wrote:
> On Wed, Apr 22, 2026 at 01:35:55PM +0000, Sebastian Ene wrote:
> > On Wed, Apr 22, 2026 at 01:24:02PM +0100, Marc Zyngier wrote:
> > > On Wed, 22 Apr 2026 11:25:40 +0100,
> > > Sebastian Ene <sebastianene@google.com> wrote:
> > > > 
> > > > Prevent the pKVM hypervisor from making assumptions that the
> > > > endpoint memory access descriptor (EMAD) comes right after the
> > > > FF-A memory region header and enforce a strict placement for it
> > > > when validating an FF-A memory lend/share transaction.
> > 
> > Hello Marc,
> > 
> > > 
> > > As I read this, you want to remove a bad assumption...
> > > 
> > > > 
> > > > Prior to FF-A version 1.1 the header of the memory region
> > > > didn't contain an offset to the endpoint memory access descriptor.
> > > > The layout of a memory transaction looks like this:
> > > > 
> > > >   Field name				| Offset
> > > > 					 -- 0
> > > > [ Header (ffa_mem_region)               |__ ep_mem_offset
> > > >   EMAD 1 (ffa_mem_region_attributes)	|
> > > > ]
> > > > 
> > > > Reject the host from specifying a memory access descriptor offset
> > > > that is different than the size of the memory region header.
> > > 
> > > And yet you decide that you want to enforce this assumption. I don't
> > > understand how you arrive to this conclusion.
> > > 
> > > Looking at the spec, it appears that the offset is *designed* to allow
> > > a gap between the header and the EMAD. Refusing to handle a it seems to be a
> > > violation of the spec.
> > > 
> > > What am I missing?
> > 
> > While the spec allows the gap to be variable (since version 1.1), the
> > arm ff-a driver places it at a fixed position in:
> > ffa_mem_region_additional_setup() 
> > https://elixir.bootlin.com/linux/v7.0/source/drivers/firmware/arm_ffa/driver.c#L671
> > 
> 

Hello Sudeep,

> That's just the current choice in the driver and can be changed in the future.
> 
> > and makes use of the same assumption in: ffa_mem_desc_offset().
> > https://elixir.bootlin.com/linux/v7.0/source/include/linux/arm_ffa.h#L448
> 
> Again this is just in the transmit path of the message the driver is
> constructing and hence it is a simple choice rather than wrong assumption.
>
> > The later one seems wrong IMO. because we should compute the offset
> > based on the value stored in ep_mem_offset and not adding it up with
> > sizeof(struct ffa_mem_region).
> > 
> 
> Sorry what am I missing as the driver is building these descriptors to
> send it across to SPMC, we are populating the field and it will be 0
> before it is initialised

Right, what I meant is having something like this since this function is not limited
to the driver scope and using it from other components would imply relying on the
assumption: 'ep_mem_offset == sizeof(struct ffa_mem_region)'. We will also have to validate
that the `ep_mem_offset` doesn't point outside of the mailbox designated buffer.

---
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 81e603839c4a..62d67dae8b70 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -445,7 +445,7 @@ ffa_mem_desc_offset(struct ffa_mem_region *buf, int count, u32 ffa_version)
        if (!FFA_MEM_REGION_HAS_EP_MEM_OFFSET(ffa_version))
                offset += offsetof(struct ffa_mem_region, ep_mem_offset);
        else
-               offset += sizeof(struct ffa_mem_region);
+               offset += buf->ep_mem_offset;
 
        return offset;
 }
---

And then move `ffa_mem_region_additional_setup` to be called earlier before `ffa_mem_desc_offset`:
(so that it can setup the value for ep_mem_offset)

---
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index f2f94d4d533e..66de59c88aff 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -691,6 +691,8 @@ ffa_setup_and_transmit(u32 func_id, void *buffer, u32 max_fragsize,
        mem_region->flags = args->flags;
        mem_region->sender_id = drv_info->vm_id;
        mem_region->attributes = ffa_memory_attributes_get(func_id);
+
+       ffa_mem_region_additional_setup(drv_info->version, mem_region);
        composite_offset = ffa_mem_desc_offset(buffer, args->nattrs,
                                               drv_info->version);
 
@@ -708,7 +710,6 @@ ffa_setup_and_transmit(u32 func_id, void *buffer, u32 max_fragsize,
        }
        mem_region->handle = 0;
        mem_region->ep_count = args->nattrs;
-       ffa_mem_region_additional_setup(drv_info->version, mem_region);
---

> 
> > Maybe this should be the fix instead and not the one in pKVM ? What do
> > you think ?
> > 
> 
> Can you share the diff you have in mind to understand your concern better
> or are you referring to this patch itself.

Sure, please let me know if you think this is wrong. I might have misunderstood it. 

> 
> > The current implementation in pKVM makes use of the
> > ffa_mem_desc_offset() to validate the first EMAD. If a compromised host
> > places an EMAD at a different offset than sizeof(struct ffa_mem_region),
> > then pKVM will not validate that EMAD.
> >
> 
> Calling the host as compromised if it chooses a different offset seems bit
> of extreme here. I am no sure if I am missing to understand something here.
> 

Sorry for not explaining it, in pKVM model we don't trust the host kernel so we can assume that
everything that doesn't pass the hypervisor validation(in this case the ff-a memory transaction)
can be a potential attack that wants to compromise EL2.

> -- 
> Regards,
> Sudeep

Thanks,
Sebastian