Re: [PATCH 02/12] xen/arm: ffa: Fix MEM_SHARE NS attribute handling

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 6 Feb 2026, at 10:28, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> The FF-A memory attribute encoding is currently a literal value (0x2f),
>> which makes reviews and validation harder. In addition, MEM_SHARE
>> accepts the NS (non-secure) attribute bit even though the normal world
>> must not set it according to FF-A specification.
>> 
>> Introduce named attribute bit masks and express FFA_NORMAL_MEM_REG_ATTR
>> in terms of them for clarity.
>> 
>> Reject MEM_SHARE descriptors with the NS bit set, returning
>> INVALID_PARAMETERS to match FF-A v1.1 rules that prohibit normal world
>> from setting this bit.
>> 
>> Functional impact: MEM_SHARE now rejects descriptors with NS bit set,
>> which were previously accepted but violate FF-A specification.
> 
> To be fair, it was also rejected earlier, but with a different error code.

True, will adapt the impact comment to say:

Functional impact: MEM_SHARE now rejects descriptors with NS bit set
with the right error code, INVALID_PARAMETER.

Tell me if that would be ok for you and if it could be fixed on commit with
your R-b if it is the case.

> 
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> xen/arch/arm/tee/ffa_private.h | 17 ++++++++++++++++-
>> xen/arch/arm/tee/ffa_shm.c     |  6 ++++++
>> 2 files changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index cd7ecabc7eff..b625f1c72914 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -129,11 +129,26 @@
>> #define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
>> #define FFA_HANDLE_INVALID              0xffffffffffffffffULL
>> 
>> +/* NS attribute was introduced in v1.1 */
>> +#define FFA_MEM_ATTR_NS                 BIT(6, U)
>> +
>> +#define FFA_MEM_ATTR_TYPE_DEV           (1U << 3)
>> +#define FFA_MEM_ATTR_TYPE_MEM           (2U << 4)
>> +
>> +#define FFA_MEM_ATTR_NC                 (1U << 2)
>> +#define FFA_MEM_ATTR_WB                 (3U << 2)
>> +
>> +#define FFA_MEM_ATTR_NON_SHARE          (0U)
>> +#define FFA_MEM_ATTR_OUT_SHARE          (2U)
>> +#define FFA_MEM_ATTR_INN_SHARE          (3U)
>> +
>> /*
>>  * Memory attributes: Normal memory, Write-Back cacheable, Inner shareable
>>  * Defined in FF-A-1.1-REL0 Table 10.18 at page 175.
>>  */
>> -#define FFA_NORMAL_MEM_REG_ATTR         0x2fU
>> +#define FFA_NORMAL_MEM_REG_ATTR         (FFA_MEM_ATTR_TYPE_MEM | \
>> +                                         FFA_MEM_ATTR_WB | \
>> +                                         FFA_MEM_ATTR_INN_SHARE)
>> /*
>>  * Memory access permissions: Read-write
>>  * Defined in FF-A-1.1-REL0 Table 10.15 at page 168.
>> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
>> index 8282bacf85d3..90800e44a86a 100644
>> --- a/xen/arch/arm/tee/ffa_shm.c
>> +++ b/xen/arch/arm/tee/ffa_shm.c
>> @@ -512,6 +512,12 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
>>     if ( ret )
>>         goto out_unlock;
>> 
>> +    if ( trans.mem_reg_attr & FFA_MEM_ATTR_NS )
>> +    {
>> +        ret = FFA_RET_INVALID_PARAMETERS;
>> +        goto out_unlock;
>> +    }
>> +
>>     if ( trans.mem_reg_attr != FFA_NORMAL_MEM_REG_ATTR )
>>     {
>>         ret = FFA_RET_NOT_SUPPORTED;
>> --
>> 2.50.1 (Apple Git-155)
>> 
> 
> Looks good, but I think the commit message needs a small update or
> clarification.

Thanks.

Cheers
Bertrand

> 
> Cheers,
> Jens