linux-kernel - Re: [v3 PATCH] iommu/arm-smmu-v3: Fix L1 stream table index calculation for 32-bit sid size

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE2F3rDH3aK-OLO6TSXgQFU+DDx6Rq+4uwLQb3WZ+CMWWEGCNQ@mail.gmail.com>
Date: Fri, 4 Oct 2024 14:14:25 -0700
From: Daniel Mentz <danielmentz@...gle.com>
To: Yang Shi <yang@...amperecomputing.com>
Cc: jgg@...pe.ca, nicolinc@...dia.com, james.morse@....com, will@...nel.org, 
	robin.murphy@....com, linux-arm-kernel@...ts.infradead.org, 
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [v3 PATCH] iommu/arm-smmu-v3: Fix L1 stream table index
 calculation for 32-bit sid size

On Fri, Oct 4, 2024 at 11:04 AM Yang Shi <yang@...amperecomputing.com> wrote:
>  static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>  {
> -       u32 size;
> +       u64 size;
>         struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
> +       u64 num_sids = arm_smmu_strtab_num_sids(smmu);
> +
> +       size = num_sids * sizeof(struct arm_smmu_ste);
> +       /* The max size for dmam_alloc_coherent() is 32-bit */

I'd remove this comment. I assume the intent here was to say that the
maximum size is 4GB (not 32 bit). I also can't find any reference to
this limitation. Where does dmam_alloc_coherent() limit the size of an
allocation to 4GB? Also, this comment might not be applicable to 64
bit platforms.

> +       if (size > SIZE_MAX)
> +               return -EINVAL;

I'm assuming this is for platforms where the range of a u64 is larger
than that of a size_t type? If we're printing an error message if an
allocation fails (i.e. "failed to allocate linear stream table (%llu
bytes)\n"), then we might also want to print an error message here.

> -       cfg->linear.num_ents = 1 << smmu->sid_bits;
> +       cfg->linear.num_ents = num_sids;

If you're worried about 32 bit platforms, then I'm wondering if this
also needs some attention. cfg->linear.num_ents is defined as an
unsigned int and num_sids could potentially be outside the range of an
unsigned int on 32 bit platforms.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 1e9952ca989f..c8ceddc5e8ef 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -853,6 +853,11 @@ struct arm_smmu_master_domain {
>         ioasid_t ssid;
>  };
>
> +static inline u64 arm_smmu_strtab_num_sids(struct arm_smmu_device *smmu)
> +{
> +       return (1ULL << smmu->sid_bits);
> +}
> +

I'm wondering if it makes sense to move this up and put it right
before arm_smmu_strtab_l1_idx(). That way, all the arm_smmu_strtab_*
functions are in one place.

On a related note, in arm_smmu_init_strtab_2lvl() we're capping the
number of l1 entries at STRTAB_MAX_L1_ENTRIES for 2 level stream
tables. I'm thinking it would make sense to limit the size of linear
stream tables for the same reasons.