iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

[PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

Posted by Daniel Mentz 1 year, 4 months ago

The function arm_smmu_init_strtab_2lvl uses the expression

((1 << smmu->sid_bits) - 1)

to calculate the largest StreamID value. However, this fails for the
maximum allowed value of SMMU_IDR1.SIDSIZE which is 32. The C standard
states:

"If the value of the right operand is negative or is greater than or
equal to the width of the promoted left operand, the behavior is
undefined."

With smmu->sid_bits being 32, the prerequisites for undefined behavior
are met.  We observed that the value of (1 << 32) is 1 and not 0 as we
initially expected.

Similar bit shift operations in arm_smmu_init_strtab_linear seem to not
be affected, because it appears to be unlikely for an SMMU to have
SMMU_IDR1.SIDSIZE set to 32 but then not support 2-level Stream tables

This issue was found by Ryan Huang <tzukui@google.com> on our team.

Fixes: ce410410f1a7 ("iommu/arm-smmu-v3: Add arm_smmu_strtab_l1/2_idx()")
Signed-off-by: Daniel Mentz <danielmentz@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 737c5b882355..b55327d6058e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3625,7 +3625,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
 	u32 l1size;
 	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
 	unsigned int last_sid_idx =
-		arm_smmu_strtab_l1_idx((1 << smmu->sid_bits) - 1);
+		arm_smmu_strtab_l1_idx((1ULL << smmu->sid_bits) - 1);
 
 	/* Calculate the L1 size, capped to the SIDSIZE. */
 	cfg->l2.num_l1_ents = min(last_sid_idx + 1, STRTAB_MAX_L1_ENTRIES);
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

Posted by Will Deacon 1 year, 4 months ago

On Tue, 01 Oct 2024 18:53:57 -0700, Daniel Mentz wrote:
> The function arm_smmu_init_strtab_2lvl uses the expression
> 
> ((1 << smmu->sid_bits) - 1)
> 
> to calculate the largest StreamID value. However, this fails for the
> maximum allowed value of SMMU_IDR1.SIDSIZE which is 32. The C standard
> states:
> 
> [...]

Applied to will (for-joerg/arm-smmu/fixes), thanks!

[1/1] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32
      https://git.kernel.org/will/c/f63237f54cf1

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

Posted by Nicolin Chen 1 year, 4 months ago

On Tue, Oct 01, 2024 at 06:53:57PM -0700, Daniel Mentz wrote:
> 
> The function arm_smmu_init_strtab_2lvl uses the expression
> 
> ((1 << smmu->sid_bits) - 1)
> 
> to calculate the largest StreamID value. However, this fails for the
> maximum allowed value of SMMU_IDR1.SIDSIZE which is 32. The C standard
> states:
> 
> "If the value of the right operand is negative or is greater than or
> equal to the width of the promoted left operand, the behavior is
> undefined."
> 
> With smmu->sid_bits being 32, the prerequisites for undefined behavior
> are met.  We observed that the value of (1 << 32) is 1 and not 0 as we
> initially expected.
> 
> Similar bit shift operations in arm_smmu_init_strtab_linear seem to not
> be affected, because it appears to be unlikely for an SMMU to have
> SMMU_IDR1.SIDSIZE set to 32 but then not support 2-level Stream tables
> 
> This issue was found by Ryan Huang <tzukui@google.com> on our team.

There is a patch that's sent a few hours earlier :)
https://lore.kernel.org/linux-arm-kernel/20241001180346.1485194-1-yang@os.amperecomputing.com/

Thanks
Nicolin

> Fixes: ce410410f1a7 ("iommu/arm-smmu-v3: Add arm_smmu_strtab_l1/2_idx()")
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 737c5b882355..b55327d6058e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3625,7 +3625,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
>         u32 l1size;
>         struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>         unsigned int last_sid_idx =
> -               arm_smmu_strtab_l1_idx((1 << smmu->sid_bits) - 1);
> +               arm_smmu_strtab_l1_idx((1ULL << smmu->sid_bits) - 1);
> 
>         /* Calculate the L1 size, capped to the SIDSIZE. */
>         cfg->l2.num_l1_ents = min(last_sid_idx + 1, STRTAB_MAX_L1_ENTRIES);
> --
> 2.46.1.824.gd892dcdcdd-goog
>

Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

Posted by Daniel Mentz 1 year, 4 months ago

On Tue, Oct 1, 2024 at 9:05 PM Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> On Tue, Oct 01, 2024 at 06:53:57PM -0700, Daniel Mentz wrote:
> >
> > The function arm_smmu_init_strtab_2lvl uses the expression
> >
> > ((1 << smmu->sid_bits) - 1)
> >
> > to calculate the largest StreamID value. However, this fails for the
> > maximum allowed value of SMMU_IDR1.SIDSIZE which is 32. The C standard
> > states:
> >
> > "If the value of the right operand is negative or is greater than or
> > equal to the width of the promoted left operand, the behavior is
> > undefined."
> >
> > With smmu->sid_bits being 32, the prerequisites for undefined behavior
> > are met.  We observed that the value of (1 << 32) is 1 and not 0 as we
> > initially expected.
> >
> > Similar bit shift operations in arm_smmu_init_strtab_linear seem to not
> > be affected, because it appears to be unlikely for an SMMU to have
> > SMMU_IDR1.SIDSIZE set to 32 but then not support 2-level Stream tables
> >
> > This issue was found by Ryan Huang <tzukui@google.com> on our team.
>
> There is a patch that's sent a few hours earlier :)
> https://lore.kernel.org/linux-arm-kernel/20241001180346.1485194-1-yang@os.amperecomputing.com/

Thanks Nicolin.

Yang, in your change, I believe you are arguing based on the Arm spec
("dest = src << (shift % 32)"). Consider mentioning that the C
standard states that this behavior is undefined.

>
> Thanks
> Nicolin
>
> > Fixes: ce410410f1a7 ("iommu/arm-smmu-v3: Add arm_smmu_strtab_l1/2_idx()")
> > Signed-off-by: Daniel Mentz <danielmentz@google.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 737c5b882355..b55327d6058e 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3625,7 +3625,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
> >         u32 l1size;
> >         struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
> >         unsigned int last_sid_idx =
> > -               arm_smmu_strtab_l1_idx((1 << smmu->sid_bits) - 1);
> > +               arm_smmu_strtab_l1_idx((1ULL << smmu->sid_bits) - 1);
> >
> >         /* Calculate the L1 size, capped to the SIDSIZE. */
> >         cfg->l2.num_l1_ents = min(last_sid_idx + 1, STRTAB_MAX_L1_ENTRIES);
> > --
> > 2.46.1.824.gd892dcdcdd-goog
> >

Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid_bits==32

Posted by Yang Shi 1 year, 4 months ago


On 10/2/24 10:58 AM, Daniel Mentz wrote:
> On Tue, Oct 1, 2024 at 9:05 PM Nicolin Chen <nicolinc@nvidia.com> wrote:
>> On Tue, Oct 01, 2024 at 06:53:57PM -0700, Daniel Mentz wrote:
>>> The function arm_smmu_init_strtab_2lvl uses the expression
>>>
>>> ((1 << smmu->sid_bits) - 1)
>>>
>>> to calculate the largest StreamID value. However, this fails for the
>>> maximum allowed value of SMMU_IDR1.SIDSIZE which is 32. The C standard
>>> states:
>>>
>>> "If the value of the right operand is negative or is greater than or
>>> equal to the width of the promoted left operand, the behavior is
>>> undefined."
>>>
>>> With smmu->sid_bits being 32, the prerequisites for undefined behavior
>>> are met.  We observed that the value of (1 << 32) is 1 and not 0 as we
>>> initially expected.
>>>
>>> Similar bit shift operations in arm_smmu_init_strtab_linear seem to not
>>> be affected, because it appears to be unlikely for an SMMU to have
>>> SMMU_IDR1.SIDSIZE set to 32 but then not support 2-level Stream tables
>>>
>>> This issue was found by Ryan Huang <tzukui@google.com> on our team.
>> There is a patch that's sent a few hours earlier :)
>> https://lore.kernel.org/linux-arm-kernel/20241001180346.1485194-1-yang@os.amperecomputing.com/
> Thanks Nicolin.
>
> Yang, in your change, I believe you are arguing based on the Arm spec
> ("dest = src << (shift % 32)"). Consider mentioning that the C
> standard states that this behavior is undefined.

OK, I will add this info to the commit log too.

>
>> Thanks
>> Nicolin
>>
>>> Fixes: ce410410f1a7 ("iommu/arm-smmu-v3: Add arm_smmu_strtab_l1/2_idx()")
>>> Signed-off-by: Daniel Mentz <danielmentz@google.com>
>>> ---
>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 737c5b882355..b55327d6058e 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -3625,7 +3625,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
>>>          u32 l1size;
>>>          struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>>>          unsigned int last_sid_idx =
>>> -               arm_smmu_strtab_l1_idx((1 << smmu->sid_bits) - 1);
>>> +               arm_smmu_strtab_l1_idx((1ULL << smmu->sid_bits) - 1);
>>>
>>>          /* Calculate the L1 size, capped to the SIDSIZE. */
>>>          cfg->l2.num_l1_ents = min(last_sid_idx + 1, STRTAB_MAX_L1_ENTRIES);
>>> --
>>> 2.46.1.824.gd892dcdcdd-goog
>>>