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

[Yang Shi]

On 10/2/24 10:58 AM, Daniel Mentz wrote:
> On Tue, Oct 1, 2024 at 9:05 PM Nicolin Chen <nicolinc@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx/
> 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@xxxxxxxxxx>
> > > ---
> > >   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