linux-kernel - Re: [v2 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: <f9933026-4756-403c-8f36-cf64fced8ff9@os.amperecomputing.com>
Date: Wed, 2 Oct 2024 13:00:24 -0700
From: Yang Shi <yang@...amperecomputing.com>
To: Jason Gunthorpe <jgg@...pe.ca>, Nicolin Chen <nicolinc@...dia.com>
Cc: james.morse@....com, will@...nel.org, robin.murphy@....com,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH] iommu/arm-smmu-v3: Fix L1 stream table index
 calculation for 32-bit sid size



On 10/2/24 12:40 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2024 at 12:22:48PM -0700, Nicolin Chen wrote:
>> On Wed, Oct 02, 2024 at 12:04:32PM -0700, Yang Shi wrote:
>>>> On Wed, Oct 02, 2024 at 10:55:14AM -0700, Yang Shi wrote:
>>>>> +static inline unsigned int arm_smmu_strtab_max_sid(struct arm_smmu_device *smmu)
>>>>> +{
>>>>> +       return (1ULL << smmu->sid_bits);
>>>>> +}
>>>>> +
>>>> Hmm, why ULL gets truncated to unsigned int here?
>>> No particular reason, but it should be better to not truncate here. Will
>>> fix it.
>> Yea, and looks like we are going to do with:
>> static inline u64 arm_smmu_strtab_num_sids(struct arm_smmu_device *smmu);
>>
>> Then let's be careful at those return-value holders too:
>> -----------------------------------------------------------
>> static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>> {
>> 	u32 size;
>> 	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
>>
>> 	size = (1 << smmu->sid_bits) * sizeof(struct arm_smmu_ste);
>>          ^^^^
>>          overflow?
>> [...]
>> 	cfg->linear.num_ents = 1 << smmu->sid_bits;
>>                      ^^^^^^^^
>>                      This is u32
>> -----------------------------------------------------------

Instead of relying in separate emails, I'd prefer reply in one single place.

This is linear stream table code, IIUC nobody actually implements such 
large sid size with linear table per the earlier discussions.

> It would make some sense to have something like:
>
>   u64 size = arm_smmu_strtab_max_sid()
>
>   /* Would require too much memory */
>   if (size > SZ_512M)
>      return -EINVAL;

This extra check is definitely fine to me and makes sense. SZ_512M is 1 
<< 29, so it means no hardware actually has 29 bit sid size with linear 
stream table. It makes sense to me. But I'm not smmu expert, so just 
would like to double check such configuration is not existing.

>
> Just to reject bad configuration rather than truncate the allocation
> and overflow STE array memory or something. Having drivers be robust
> to this kind of stuff is a confidential compute topic :\
>
> Jason