[PATCH 0/2] Sanitize SMMU_S_STRTAB_BASE_CFG LOG2SIZE and SPLIT

[PATCH 0/2] Sanitize SMMU_S_STRTAB_BASE_CFG LOG2SIZE and SPLIT

[Eric Auger]

The current code fails to sanitize SMMU_S_STRTAB_BASE_CFG
LOG2SIZE and SPLIT which are used in the STE lookup. This
could potentially lead to wrong shifts/masks in
smmu_find_ste() and does not fully comply with the spec.

Best Regards

Eric

Eric Auger (2):
  hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.SPLIT
  hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

 hw/arm/smmuv3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.53.0

[PATCH 1/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.SPLIT

[Eric Auger]

Currently the guest value for the SPLIT field is not checked.
Also the spec says that values different from 6, 8, 10, respectively
meaning 4KB, 16kB and 64kB leaf tables are reserved and behave as 6.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 5e5a6a960c9..0a8d2fbb6a7 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1688,6 +1688,13 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
         s->strtab_base_cfg = data;
         if (FIELD_EX32(data, STRTAB_BASE_CFG, FMT) == 1) {
             s->sid_split = FIELD_EX32(data, STRTAB_BASE_CFG, SPLIT);
+            if (s->sid_split != 6 && s->sid_split != 8 && s->sid_split != 10) {
+                /* Other values are reserved, behave as 6 */
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "Invalid STRTAB_BASE_CFG.SPLIT=0x%b, use 0b0110\n",
+                              s->sid_split);
+                s->sid_split = 6;
+            }
             s->features |= SMMU_FEATURE_2LVL_STE;
         }
         break;
-- 
2.53.0

RE: [PATCH 1/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.SPLIT

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 30 June 2026 09:30
> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
> berrange@redhat.com; Shameer Kolothum Thodi
> <skolothumtho@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; Nathan
> Chen <nathanc@nvidia.com>
> Subject: [PATCH 1/2] hw/arm/smmuv3: Sanitize
> SMMU_S_STRTAB_BASE_CFG.SPLIT
> 
> External email: Use caution opening links or attachments
> 
> 
> Currently the guest value for the SPLIT field is not checked.
> Also the spec says that values different from 6, 8, 10, respectively
> meaning 4KB, 16kB and 64kB leaf tables are reserved and behave as 6.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 5e5a6a960c9..0a8d2fbb6a7 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1688,6 +1688,13 @@ static MemTxResult smmu_writel(SMMUv3State
> *s, hwaddr offset,
>          s->strtab_base_cfg = data;
>          if (FIELD_EX32(data, STRTAB_BASE_CFG, FMT) == 1) {
>              s->sid_split = FIELD_EX32(data, STRTAB_BASE_CFG, SPLIT);
> +            if (s->sid_split != 6 && s->sid_split != 8 && s->sid_split != 10) {
> +                /* Other values are reserved, behave as 6 */
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "Invalid STRTAB_BASE_CFG.SPLIT=0x%b, use 0b0110\n",
> +                              s->sid_split);

0x%b is confusing. I think either 0b%b or %u is better.

Other than that,

Reviewed-by: Shameer Kolothum <skolothumtho@nvidia.com>

Thanks,
Shameer

Re: [PATCH 1/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.SPLIT

[Eric Auger]

Hi Shameer,

On 6/30/26 11:11 AM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 30 June 2026 09:30
>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
>> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
>> berrange@redhat.com; Shameer Kolothum Thodi
>> <skolothumtho@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; Nathan
>> Chen <nathanc@nvidia.com>
>> Subject: [PATCH 1/2] hw/arm/smmuv3: Sanitize
>> SMMU_S_STRTAB_BASE_CFG.SPLIT
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Currently the guest value for the SPLIT field is not checked.
>> Also the spec says that values different from 6, 8, 10, respectively
>> meaning 4KB, 16kB and 64kB leaf tables are reserved and behave as 6.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 5e5a6a960c9..0a8d2fbb6a7 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -1688,6 +1688,13 @@ static MemTxResult smmu_writel(SMMUv3State
>> *s, hwaddr offset,
>>          s->strtab_base_cfg = data;
>>          if (FIELD_EX32(data, STRTAB_BASE_CFG, FMT) == 1) {
>>              s->sid_split = FIELD_EX32(data, STRTAB_BASE_CFG, SPLIT);
>> +            if (s->sid_split != 6 && s->sid_split != 8 && s->sid_split != 10) {
>> +                /* Other values are reserved, behave as 6 */
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                              "Invalid STRTAB_BASE_CFG.SPLIT=0x%b, use 0b0110\n",
>> +                              s->sid_split);
> 0x%b is confusing. I think either 0b%b or %u is better.
sure replaced by
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "Invalid STRTAB_BASE_CFG.SPLIT=%u, use 6
instead\n",

Thanks

Eric
>
> Other than that,
>
> Reviewed-by: Shameer Kolothum <skolothumtho@nvidia.com>
>
> Thanks,
> Shameer
>

[PATCH 2/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

[Eric Auger]

STRTAB_BASE_CFG.LOG2SIZE is programmed by the guest through the
emulated SMMUv3 MMIO register interface. Currently the value is
not checked.

The SMMU spec says: "Except for readback of a written value, the
effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of
input StreamID range checking and upper/lower/linear Stream table
index address calculation."

So for STE lookup make sure log2size is capped at SMMU_IDR1.SIDSIZE.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 0a8d2fbb6a7..db5aca26ec0 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -672,6 +672,15 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, SMMUEventInfo *event)
     /*
      * Check SID range against both guest-configured and implementation limits
      */
+    if (log2size > SMMU_IDR1_SIDSIZE) {
+        /*
+         * spec says: Except for readback of a written value, the effective
+         * LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input StreamID
+         * range checking and upper/lower/linear Stream table index address
+         * calculation.
+         */
+        log2size = SMMU_IDR1_SIDSIZE;
+    }
     if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
         event->type = SMMU_EVT_C_BAD_STREAMID;
         return -EINVAL;
-- 
2.53.0

Re: [PATCH 2/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

[Daniel P. Berrangé]

On Tue, Jun 30, 2026 at 10:30:20AM +0200, Eric Auger wrote:
> STRTAB_BASE_CFG.LOG2SIZE is programmed by the guest through the
> emulated SMMUv3 MMIO register interface. Currently the value is
> not checked.
> 
> The SMMU spec says: "Except for readback of a written value, the
> effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of
> input StreamID range checking and upper/lower/linear Stream table
> index address calculation."
> 
> So for STE lookup make sure log2size is capped at SMMU_IDR1.SIDSIZE.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Can you add the "Resolves: bug URL" to the commit message so it
gets auto-closed on merge.

> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 0a8d2fbb6a7..db5aca26ec0 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -672,6 +672,15 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, SMMUEventInfo *event)
>      /*
>       * Check SID range against both guest-configured and implementation limits
>       */
> +    if (log2size > SMMU_IDR1_SIDSIZE) {
> +        /*
> +         * spec says: Except for readback of a written value, the effective
> +         * LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input StreamID
> +         * range checking and upper/lower/linear Stream table index address
> +         * calculation.
> +         */
> +        log2size = SMMU_IDR1_SIDSIZE;
> +    }
>      if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
>          event->type = SMMU_EVT_C_BAD_STREAMID;
>          return -EINVAL;
> -- 
> 2.53.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 2/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

[Eric Auger]

On 6/30/26 11:18 AM, Daniel P. Berrangé wrote:
> On Tue, Jun 30, 2026 at 10:30:20AM +0200, Eric Auger wrote:
>> STRTAB_BASE_CFG.LOG2SIZE is programmed by the guest through the
>> emulated SMMUv3 MMIO register interface. Currently the value is
>> not checked.
>>
>> The SMMU spec says: "Except for readback of a written value, the
>> effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of
>> input StreamID range checking and upper/lower/linear Stream table
>> index address calculation."
>>
>> So for STE lookup make sure log2size is capped at SMMU_IDR1.SIDSIZE.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
> Can you add the "Resolves: bug URL" to the commit message so it
> gets auto-closed on merge.

sure. Thanks for the hint

Eric
>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 0a8d2fbb6a7..db5aca26ec0 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -672,6 +672,15 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, SMMUEventInfo *event)
>>      /*
>>       * Check SID range against both guest-configured and implementation limits
>>       */
>> +    if (log2size > SMMU_IDR1_SIDSIZE) {
>> +        /*
>> +         * spec says: Except for readback of a written value, the effective
>> +         * LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input StreamID
>> +         * range checking and upper/lower/linear Stream table index address
>> +         * calculation.
>> +         */
>> +        log2size = SMMU_IDR1_SIDSIZE;
>> +    }
>>      if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
>>          event->type = SMMU_EVT_C_BAD_STREAMID;
>>          return -EINVAL;
>> -- 
>> 2.53.0
>>
> With regards,
> Daniel

RE: [PATCH 2/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 30 June 2026 09:30
> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
> berrange@redhat.com; Shameer Kolothum Thodi
> <skolothumtho@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; Nathan
> Chen <nathanc@nvidia.com>
> Subject: [PATCH 2/2] hw/arm/smmuv3: Sanitize
> SMMU_S_STRTAB_BASE_CFG.LOG2SIZE
> 
> External email: Use caution opening links or attachments
> 
> 
> STRTAB_BASE_CFG.LOG2SIZE is programmed by the guest through the
> emulated SMMUv3 MMIO register interface. Currently the value is
> not checked.
> 
> The SMMU spec says: "Except for readback of a written value, the
> effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of
> input StreamID range checking and upper/lower/linear Stream table
> index address calculation."
> 
> So for STE lookup make sure log2size is capped at SMMU_IDR1.SIDSIZE.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 0a8d2fbb6a7..db5aca26ec0 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -672,6 +672,15 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid,
> STE *ste, SMMUEventInfo *event)
>      /*
>       * Check SID range against both guest-configured and implementation limits
>       */
> +    if (log2size > SMMU_IDR1_SIDSIZE) {
> +        /*
> +         * spec says: Except for readback of a written value, the effective
> +         * LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input
> StreamID
> +         * range checking and upper/lower/linear Stream table index address
> +         * calculation.
> +         */
> +        log2size = SMMU_IDR1_SIDSIZE;

We are now limiting the log2size here and later in the code we use
this for strtab_size_shift calculation.

However, spec says(6.3.24):

"The alignment of ADDR is affected by the literal value of the respective
SMMU_STRTAB_BASE_CFG.LOG2SIZE field and is not limited by SIDSIZE."

So this will change the behaviour now. Please check.

> +    }
>      if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {

This MIN check now is redundant here, right?

Thanks,
Shameer

Re: [PATCH 2/2] hw/arm/smmuv3: Sanitize SMMU_S_STRTAB_BASE_CFG.LOG2SIZE

[Eric Auger]

Hi Shameer,

On 6/30/26 11:43 AM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 30 June 2026 09:30
>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
>> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
>> berrange@redhat.com; Shameer Kolothum Thodi
>> <skolothumtho@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; Nathan
>> Chen <nathanc@nvidia.com>
>> Subject: [PATCH 2/2] hw/arm/smmuv3: Sanitize
>> SMMU_S_STRTAB_BASE_CFG.LOG2SIZE
>>
>> External email: Use caution opening links or attachments
>>
>>
>> STRTAB_BASE_CFG.LOG2SIZE is programmed by the guest through the
>> emulated SMMUv3 MMIO register interface. Currently the value is
>> not checked.
>>
>> The SMMU spec says: "Except for readback of a written value, the
>> effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of
>> input StreamID range checking and upper/lower/linear Stream table
>> index address calculation."
>>
>> So for STE lookup make sure log2size is capped at SMMU_IDR1.SIDSIZE.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 0a8d2fbb6a7..db5aca26ec0 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -672,6 +672,15 @@ int smmu_find_ste(SMMUv3State *s, uint32_t sid,
>> STE *ste, SMMUEventInfo *event)
>>      /*
>>       * Check SID range against both guest-configured and implementation limits
>>       */
>> +    if (log2size > SMMU_IDR1_SIDSIZE) {
>> +        /*
>> +         * spec says: Except for readback of a written value, the effective
>> +         * LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input
>> StreamID
>> +         * range checking and upper/lower/linear Stream table index address
>> +         * calculation.
>> +         */
>> +        log2size = SMMU_IDR1_SIDSIZE;
> We are now limiting the log2size here and later in the code we use
> this for strtab_size_shift calculation.
>
> However, spec says(6.3.24):
>
> "The alignment of ADDR is affected by the literal value of the respective
> SMMU_STRTAB_BASE_CFG.LOG2SIZE field and is not limited by SIDSIZE."
>
> So this will change the behaviour now. Please check.

You right, there is this statement  + the existing comment in the code +
the additional note: "Note: This means that configuring a table that is
larger than required by the incoming StreamID span results
in some entries being unreachable, but the table is still aligned to the
configured size."

and then there is

"../.. the effective LOG2SIZE is <= SMMU_IDR1.SIDSIZE for the purposes of input
StreamID range checking and upper/lower/linear Stream table index address
calculation.
"

This sounds contradictory to me.

On the other hand who is going to program an LOG2SIZE greater than the SIDSIZE?

There is no further info about log2size limit and with some of its values spec ADDR alignment computations cannot apply

ADDR[LOG2SIZE + 5:0] = 0.
ADDR[MAX(5, (LOG2SIZE - SPLIT - 1 + 3)):0] = 0.


Adding to that, the code currently does not consider SPLIT >= LOG2SIZE case and also the ignores aligment computation of L2PTR according to span.

I will do my best to rework that ...

Thanks

Eric


>
>> +    }
>>      if (sid >= (1 << MIN(log2size, SMMU_IDR1_SIDSIZE))) {
> This MIN check now is redundant here, right?
Yes it is
>
> Thanks,
> Shameer
>