arch/arm64/kvm/hyp/nvhe/ffa.c | 6 ++++++ 1 file changed, 6 insertions(+)
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.
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.
Cc: stable@vger.kernel.org
Fixes: 42fb33dde42b ("KVM: arm64: Use FF-A 1.1 with pKVM")
Signed-off-by: Sebastian Ene <sebastianene@google.com>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 94161ea1cd60..0703c0ad8dff 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -508,6 +508,12 @@ static void __do_ffa_mem_xfer(const u64 func_id,
buf = hyp_buffers.tx;
memcpy(buf, host_buffers.tx, fraglen);
+ if (FFA_MEM_REGION_HAS_EP_MEM_OFFSET(hyp_ffa_version) &&
+ buf->ep_mem_offset != sizeof(struct ffa_mem_region)) {
+ ret = FFA_RET_INVALID_PARAMETERS;
+ goto out_unlock;
+ }
+
ep_mem_access = (void *)buf +
ffa_mem_desc_offset(buf, 0, hyp_ffa_version);
offset = ep_mem_access->composite_off;
--
2.54.0.rc1.555.g9c883467ad-googI haven't tested this, but the change looks reasonable to me.
Samet
> 2026. 4. 22. 오후 1:27, Sebastian Ene <sebastianene@google.com> 작성:
>
> 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.
>
> 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.
>
> Cc: stable@vger.kernel.org
> Fixes: 42fb33dde42b ("KVM: arm64: Use FF-A 1.1 with pKVM")
> Signed-off-by: Sebastian Ene <sebastianene@google.com>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 94161ea1cd60..0703c0ad8dff 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -508,6 +508,12 @@ static void __do_ffa_mem_xfer(const u64 func_id,
> buf = hyp_buffers.tx;
> memcpy(buf, host_buffers.tx, fraglen);
>
> + if (FFA_MEM_REGION_HAS_EP_MEM_OFFSET(hyp_ffa_version) &&
> + buf->ep_mem_offset != sizeof(struct ffa_mem_region)) {
> + ret = FFA_RET_INVALID_PARAMETERS;
> + goto out_unlock;
> + }
> +
> ep_mem_access = (void *)buf +
> ffa_mem_desc_offset(buf, 0, hyp_ffa_version);
> offset = ep_mem_access->composite_off;
> --
> 2.54.0.rc1.555.g9c883467ad-goog
>
>
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. 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? M. -- Without deviation from the norm, progress is not possible.
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. > > As I read this, you want to remove a bad assumption... > Indeed, it matches my understanding as well. I got confused with the code change initially only to realise you want to restrict the choice of offset. > > > > 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. > +1 -- Regards, Sudeep
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 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 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). Maybe this should be the fix instead and not the one in pKVM ? What do you think ? 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. > > M. > > -- > Without deviation from the norm, progress is not possible. Thanks, Sebastian
On Wed, 22 Apr 2026 14:35:55 +0100, Sebastian Ene <sebastianene@google.com> 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 That's an implementation detail, and you shouldn't rely on this. > 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 > 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). > > Maybe this should be the fix instead and not the one in pKVM ? What do > you think ? I think you should parse the buffers as the spec intends them, without assumptions or limitations. > > 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. Why compromised? Isn't that a perfectly valid thing to do? What I understand is that the FFA 1.1 implementation in pKVM doesn't match the expectations of the spec. If that's indeed the case, pKVM should be fixed to accept these messages correctly, or stop using FFA 1.1. M. -- Without deviation from the norm, progress is not possible.
On Thu, Apr 23, 2026 at 09:08:46AM +0100, Marc Zyngier wrote: > On Wed, 22 Apr 2026 14:35:55 +0100, > Sebastian Ene <sebastianene@google.com> 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 > > That's an implementation detail, and you shouldn't rely on this. > > > 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 > > 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). > > > > Maybe this should be the fix instead and not the one in pKVM ? What do > > you think ? > > I think you should parse the buffers as the spec intends them, without > assumptions or limitations. Ack. > > > > > 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. > > Why compromised? Isn't that a perfectly valid thing to do? What I > understand is that the FFA 1.1 implementation in pKVM doesn't match > the expectations of the spec. If that's indeed the case, pKVM should > be fixed to accept these messages correctly, or stop using FFA 1.1. > > M. Sorry, what I meant is that a potentially malicious host could abuse this limitation of the FF-A proxy validation which is looking at a fixed offset to do the EMAD validation. Another EMAD can be placed at a different offset and it will bypass the validation of the proxy alltogether. We have two choices: the simple one is what this patch does (enforce a fixed offset) or the second one : patch `ffa_mem_desc_offset` to use ep_mem_offset instead of `sizeof(struct ffa_mem_region)` and validate the ep_mem_offset. > > -- > Without deviation from the norm, progress is not possible. Thanks, Sebastian
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 > 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 > 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. > 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. -- Regards, Sudeep
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
On Thu, Apr 23, 2026 at 09:17:49AM +0000, Sebastian Ene wrote: > On Wed, Apr 22, 2026 at 08:29:06PM +0100, Sudeep Holla wrote: [...] > 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. > Sure, we can extend the function itself or add addition helper to get the functionality you are looking for the validation. > --- > 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); Ah this could do the trick. I need to check if all the usages are covered though. > 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. > Nope, the patch helped to understand it quicker. Thanks for that. > > > > > 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. > I am aware of the principle in general, but this example with different offset can't be assumed as comprised host if the offset + size is well within the Tx buffer size boundaries. That should be the way for you to cross check for any compromise IHMO. -- Regards, Sudeep
On Thu, Apr 23, 2026 at 10:55:34AM +0100, Sudeep Holla wrote: > On Thu, Apr 23, 2026 at 09:17:49AM +0000, Sebastian Ene wrote: > > On Wed, Apr 22, 2026 at 08:29:06PM +0100, Sudeep Holla wrote: > > [...] > > > 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. > > > > Sure, we can extend the function itself or add addition helper to get the > functionality you are looking for the validation. > Thanks, would it be ok to BUG_ON if the offset is out of range here ? (we would probably have to pass the size of the buf as well in this function) > > --- > > 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); > > Ah this could do the trick. I need to check if all the usages are covered > though. > I looked a bit at the call paths and I think we can use it like this. Please let me know if you found it differently. I would like to re-spin another version of this patch. > > 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. > > > > Nope, the patch helped to understand it quicker. Thanks for that. > > > > > > > > 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. > > > > I am aware of the principle in general, but this example with different offset > can't be assumed as comprised host if the offset + size is well within the > Tx buffer size boundaries. That should be the way for you to cross check for > any compromise IHMO. > I agree, it cannot be assumed as a compromised host it can be perferctly normal with another driver that places it at a different offset; that's why I suggested patching ffa_mem_desc_offset instead and doing the ep_mem_offset validation there. > -- > Regards, > Sudeep Thanks, Sebastian
© 2016 - 2026 Red Hat, Inc.