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
