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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2022 12:10, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 10:30:33AM +0200, 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>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- 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));
> 
> Seeing the discussion from Andrews reply, nr_pages might be more
> appropriate while still quite short.

Yes and no - it then would be ambiguous as to what size pages are
meant.

> I'm not making my Rb conditional to that change though.

Good, thanks. But I guess I'm still somewhat stuck unless hearing
back from Andrew (although one might not count a conditional R-b
as a "pending objection"). I'll give him a few more days, but I
continue to think this ought to be a separate change (if renaming
is really needed in the 1st place) ...

Jan