Messages in this thread |  | | Date | Mon, 27 Apr 2026 11:36:23 +0000 | | From | Sebastian Ene <> | | Subject | Re: [PATCH] KVM: arm64: Validate the FF-A memory access descriptor placement |
| |
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
|  |