Hi Tao,
On 8/13/26 6:26 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 gated by whether
> SECURE_IMPL is enabled. When it is disabled, all SMMU_S_* registers are
> read-as-zero and write-ignored.
>
> 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 improves compatibility with Arm security space
> handling.
>
> Signed-off-by: Tao Tang <[email protected]>
> Reviewed-by: Pierrick Bouvier <[email protected]>
> ---
> hw/arm/smmuv3.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index dc3fa618883..e5f0bc18415 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1605,6 +1605,12 @@ static bool smmu_eventq_irq_cfg_writable(SMMUv3State
> *s, SMMUSecSID sec_sid)
> return smmu_irq_cfg_writable(s, sec_sid, SMMU_IRQ_EVTQ);
> }
>
> +/* 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, Error **errp, SMMUSecSID
> sec_sid)
> {
> SMMUState *bs = ARM_SMMU(s);
> @@ -1905,6 +1911,38 @@ static int smmuv3_cmdq_consume(SMMUv3State *s, Error
> **errp, SMMUSecSID sec_sid)
> return 0;
> }
>
> +/*
> + * Helper function for Secure register access validation.
> + *
> + * Follow S_IDR1.SECURE_IMPL accessibility rules for SMMU_S_*:
> + * - SECURE_IMPL == 0: Secure state is not implemented; SMMU_S_* are RAZ/WI
> to
> + * all accesses.
> + * - SECURE_IMPL == 1: Non-secure accesses to SMMU_S_* are RAZ/WI.
> + */
> +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",
would suggest
"%s: Non-secure %s at secure offset 0x%" PRIx64 " (%s)\n"
I am not sure we shall detail the end behavior here but rather put that in the
call site
> + __func__, is_read ? "read" : "write", offset,
> + is_read ? "RAZ" : "WI");
> + return false;
> + }
> +
> + /* Check if the secure state is implemented. */
> + if (!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);
"%s: Secure %s at secure offset 0x%" PRIx64 "without support of secure state\n"
RES0 is a bit ambiguous. is it the capability which is RES0 or is it some end
behavior associated to the access? I would skip this and detail in the caller
instead
+ * - SECURE_IMPL == 0: Secure state is not implemented; SMMU_S_* are RAZ/WI to
+ * all accesses.
> + return false;
> + }
> + return true;
> +}
> +
> static MemTxResult smmu_writell(SMMUv3State *s, hwaddr offset,
> uint64_t data, MemTxAttrs attrs,
> SMMUSecSID reg_sec_sid)
> @@ -2272,6 +2310,18 @@ static MemTxResult smmu_write_mmio(void *opaque,
> hwaddr offset, uint64_t data,
> * translate the Secure window to its bank-local register offsets.
> */
> if (offset >= SMMU_SECURE_REG_START) {
> + if (!smmu_check_secure_access(s, attrs, offset, false)) {
> + trace_smmuv3_write_mmio(offset, data, size, MEMTX_OK);
> + /*
> + * RAZ/WI/RES0 are deterministic register-level behaviors and do
> not
> + * imply a bus protocol error or abort. Therefore we acknowledge
> the
> + * MMIO transaction with MEMTX_OK and implement
> + * "Read-As-Zero / Write-Ignored" in the register model, instead
> of
> + * returning MEMTX_*_ERROR which is reserved for real
> decode/access
> + * failures.
So here you document again the associated end behavior. I think si could
be simplied to "WI single line", no?
> + */
> + return MEMTX_OK;
> + }
> reg_sec_sid = SMMU_SEC_SID_S;
> offset -= SMMU_SECURE_REG_START;
> }
> @@ -2508,6 +2558,11 @@ static MemTxResult smmu_read_mmio(void *opaque, hwaddr
> offset, uint64_t *data,
> /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */
> offset &= ~0x10000;
> 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;
> + }
> reg_sec_sid = SMMU_SEC_SID_S;
> offset -= SMMU_SECURE_REG_START;
> }
Thanks
Eric
