linux-kernel - Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for sid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZvzGg0P5Bj2sgNZm@Asurada-Nvidia>
Date: Tue, 1 Oct 2024 21:05:23 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Daniel Mentz <danielmentz@...gle.com>
CC: <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>, Jason Gunthorpe
	<jgg@...dia.com>, Ryan Huang <tzukui@...gle.com>, Will Deacon
	<will@...nel.org>, Mostafa Saleh <smostafa@...gle.com>
Subject: Re: [PATCH] iommu/arm-smmu-v3: Fix last_sid_idx calculation for
 sid_bits==32

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@...gle.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@...gle.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
>