Re: [PATCH v4 01/21] AMD/IOMMU: correct potentially-UB shifts

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2022 15:08, Andrew Cooper wrote:
> On 25/04/2022 09:30, Jan Beulich wrote:
>> Recent changes (likely 5fafa6cf529a ["AMD/IOMMU: have callers specify
>> the target level for page table walks"]) have made Coverity notice a
>> shift count in iommu_pde_from_dfn() which might in theory grow too
>> large. While this isn't a problem in practice, address the concern
>> nevertheless to not leave dangling breakage in case very large
>> superpages would be enabled at some point.
>>
>> Coverity ID: 1504264
>>
>> While there also address a similar issue in set_iommu_ptes_present().
>> It's not clear to me why Coverity hasn't spotted that one.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v4: New.
>>
>> --- a/xen/drivers/passthrough/amd/iommu_map.c
>> +++ b/xen/drivers/passthrough/amd/iommu_map.c
>> @@ -89,11 +89,11 @@ static unsigned int set_iommu_ptes_prese
>>                                             bool iw, bool ir)
>>  {
>>      union amd_iommu_pte *table, *pde;
>> -    unsigned int page_sz, flush_flags = 0;
>> +    unsigned long page_sz = 1UL << (PTE_PER_TABLE_SHIFT * (pde_level - 1));
> 
> There's an off-by-12 error somewhere here.
> 
> Judging by it's use, it should be named mapping_frames (or similar) instead.

Hmm, I think the author meant "size of the (potentially large) page
in units of 4k (base) pages". That's still some form of "page size".

> With that fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

If anything there could be another patch renaming the variable; that's
certainly not the goal here. But as said, I don't think the variable
name is strictly wrong. And with that it also doesn't feel entirely
right that I would be on the hook of renaming it. I also think that
"mapping_frames" isn't much better; it would need to be something
like "nr_frames_per_pte", which is starting to get longish.

So for the moment thanks for the R-b, but I will only apply it once
we've sorted the condition you provided it under.

Jan