Re: [RFC v3 18/21] hw/arm/smmuv3: Harden security checks in MMIO handlers

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 12/5/25 2:36 AM, Tao Tang wrote:
> Hi Eric,
> 
> On 2025/12/4 22:59, Eric Auger wrote:
>>
>> On 10/12/25 5:14 PM, Tao Tang wrote:
>>> This patch hardens the security validation within the main MMIO
>>> dispatcher functions (smmu_read_mmio and smmu_write_mmio).
>>>
>>> First, accesses to the secure register space are now correctly gated by
>>> whether the SECURE_IMPL feature is enabled in the model. This prevents
>>> guest software from accessing the secure programming interface when it is
>>> disabled, though some registers are exempt from this check as per the
>>> architecture.
>>>
>>> Second, the check for the input stream's security is made more robust.
>>> It now validates not only the legacy MemTxAttrs.secure bit, but also
>>> the .space field. This brings the SMMU's handling of security spaces
>>> into full alignment with the PE.
>>>
>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>> ---
>>>    hw/arm/smmuv3.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 64 insertions(+)
>>>
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 4ac7a2f3c7..c9c742c80b 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -1458,6 +1458,12 @@ static bool smmu_eventq_irq_cfg_writable(SMMUv3State *s, SMMUSecSID sec_sid)
>>>        return smmu_irq_ctl_evtq_irqen_disabled(s, sec_sid);
>>>    }
>>>    
>>> +/* Check if the SMMU hardware itself implements secure state features */
>>> +static inline bool smmu_hw_secure_implemented(SMMUv3State *s)
>>> +{
>>> +    return FIELD_EX32(s->bank[SMMU_SEC_SID_S].idr[1], S_IDR1, SECURE_IMPL);
>>> +}
>>> +
>>>    static int smmuv3_cmdq_consume(SMMUv3State *s, SMMUSecSID sec_sid)
>>>    {
>>>        SMMUState *bs = ARM_SMMU(s);
>>> @@ -1712,6 +1718,55 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, SMMUSecSID sec_sid)
>>>        return 0;
>>>    }
>>>    
>>> +/*
>>> + * Check if a register is exempt from the secure implementation check.
>>> + *
>>> + * The SMMU architecture specifies that certain secure registers, such as
>>> + * the secure Event Queue IRQ configuration registers, must be accessible
>>> + * even if the full secure hardware is not implemented. This function
>>> + * identifies those registers.
>>> + *
>>> + * Returns true if the register is exempt, false otherwise.
>>> + */
>>> +static bool is_secure_impl_exempt_reg(hwaddr offset)
>>> +{
>>> +    switch (offset) {
>>> +    case A_S_EVENTQ_IRQ_CFG0:
>>> +    case A_S_EVENTQ_IRQ_CFG1:
>>> +    case A_S_EVENTQ_IRQ_CFG2:
>>> +        return true;
>>> +    default:
>>> +        return false;
>>> +    }
>>> +}
>>> +
>>> +/* Helper function for Secure register access validation */
>> I think we shall improve the doc commennt for the function. I understand
>> @offset is a secure register offset and the function returns whether the
>> access to the secure register is possible. This requires a) the access
>> to be secure and in general secure state support exccet for few regs?
>>> +static bool smmu_check_secure_access(SMMUv3State *s, MemTxAttrs attrs,
>>> +                                     hwaddr offset, bool is_read)
>>> +{   /* Check if the access is secure */
>>> +    if (!(attrs.space == ARMSS_Secure ||
>>> +          attrs.secure == 1)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +            "%s: Non-secure %s attempt at offset 0x%" PRIx64 " (%s)\n",
>>> +            __func__, is_read ? "read" : "write", offset,
>>> +            is_read ? "RAZ" : "WI");
>>> +        return false;
>>> +    }
>>> +
>>> +    /*
>>> +     * Check if the secure state is implemented. Some registers are exempted
>>> +     * from this check.
>>> +     */
>>> +    if (!is_secure_impl_exempt_reg(offset) && !smmu_hw_secure_implemented(s)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +            "%s: Secure %s attempt at offset 0x%" PRIx64 ". But Secure state "
>>> +            "is not implemented (RES0)\n",
>>> +            __func__, is_read ? "read" : "write", offset);
>>> +        return false;
>>> +    }
>>> +    return true;
>>> +}
>>> +
>>>    static MemTxResult smmu_writell(SMMUv3State *s, hwaddr offset,
>>>                                    uint64_t data, MemTxAttrs attrs,
>>>                                    SMMUSecSID reg_sec_sid)
>>> @@ -2058,6 +2113,10 @@ static MemTxResult smmu_write_mmio(void *opaque, hwaddr offset, uint64_t data,
>>>         * statement to handle those specific security states.
>>>         */
>>>        if (offset >= SMMU_SECURE_REG_START) {
>>> +        if (!smmu_check_secure_access(s, attrs, offset, false)) {
>>> +            trace_smmuv3_write_mmio(offset, data, size, MEMTX_OK);
>>> +            return MEMTX_OK;
>> so the access to @offset is not permitted and we return MEMTX_OK? I am
>> confused
>>> +        }
>>>            reg_sec_sid = SMMU_SEC_SID_S;
>>>        }
>>>    
>>> @@ -2248,6 +2307,11 @@ static MemTxResult smmu_read_mmio(void *opaque, hwaddr offset, uint64_t *data,
>>>        offset &= ~0x10000;
>>>        SMMUSecSID reg_sec_sid = SMMU_SEC_SID_NS;
>>>        if (offset >= SMMU_SECURE_REG_START) {
>>> +        if (!smmu_check_secure_access(s, attrs, offset, true)) {
>>> +            *data = 0;
>>> +            trace_smmuv3_read_mmio(offset, *data, size, MEMTX_OK);
>>> +            return MEMTX_OK;
>> same here?
>>> +        }
>>>            reg_sec_sid = SMMU_SEC_SID_S;
>>>        }
>>>    
>> Thanks
>>
>> Eric
> 
> 
> Thanks for the review and for calling out the confusion around this helper.
> 
> 
> The function `smmu_check_secure_access` and `return MEMTX_OK` statement
> after smmu_check_secure_access returned false is trying to follow the
> SECURE_IMPL rules from the architecture spec:
> 
> ARM IHI 0070 G.b , 6.2 Register overview
> 
> - When SMMU_S_IDR1.SECURE_IMPL == 1, SMMU_S_* registers are RAZ/WI to
> Non-secure access. See section 3.11 Reset, Enable and initialization
> regarding Non-secure access to SMMU_S_INIT. All other registers are
> accessible to both Secure and Non-secure accesses.
> - When SMMU_S_IDR1.SECURE_IMPL == 0, SMMU_S_* registers are RES0.
>

May be worth to add a comment quoting the spec, we have been two people 
to ask the same thing. It definitely looks like a mistake when reading 
for the first time.

> 
> So the MEMTX_OK in the MMIO handlers is deliberate: we are acknowledging
> the bus transaction while applying the architectural RAZ/WI/RES0
> semantics at the register level, rather than modelling a bus abort. Also
> there was another discussion about this issue [1]  in V1 series.
> 
> [1]
>    https://lore.kernel.org/qemu-devel/a5154459-a632-42b0-b599-d5dff85b5dd2@phytium.com.cn/
> 
> 
> I'll add these needed details and parameters introduction as comment
> in `smmu_check_secure_access` and `return MEMTX_OK`. How do you think
> about it?
> 
> 
> Thanks again for the feedback,
> 
> Tao
>