Re: [PATCH] xen/arm: fix iomem_ranges cfg in map_range_to_domain()

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 14.02.25 15:15, Julien Grall wrote:
> Hi,
>
> On 14/02/2025 12:55, Grygorii Strashko wrote:
> > Now the following code in map_range_to_domain()
> >
> > res = iomem_permit_access(d, paddr_to_pfn(addr),
> >                  paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> >
> > and
> >
> > res = rangeset_add_range(mr_data->iomem_ranges,
> >                           paddr_to_pfn(addr),
> >                           paddr_to_pfn_aligned(addr + len - 1));
>  > > will incorrectly calculate end address of the iomem_range by rounding it up
> > to the next Xen page, which in turn will give Control domain access to
> > manage incorrect MMIO range.
> I think the key point that needs to be spelled out is that both functions are expecting 
> "end" to be inclusive.
 Whereas the code is thinking that the end should be exclusive.
This is a very common error in Xen and why I have been advocating several times 
to switch to use
> "start, nr" rather than "start, end".
"
Now the following code in map_range_to_domain()
...

calculates iomem_range end address by rounding it up to the next Xen page
with incorrect assumption that iomem_range end address passed to 
iomem_permit_access()/rangeset_add_range()
is exclusive, while it is expected to be inclusive.
It gives Control domain (Dom0) access to manage incorrect MMIO range with one 
additional page.
"

I can change it as above. Is it ok?

> > For example, requested range:
> >    00e6140000 - 00e6141004 should set e6140:e6141 nr=2, but will configure
> > e6140 e6142 nr=3 instead.
> I am not sure what 'nr' is referring to here.
Sorry, will change to "num_pages"?

> Also, with the range you provide above, it means that you will still give access to more than necessary. 
That's because you give access to the full page but only the first four bytes 
are valid. Is this intended?

It's known and expected for Dom0 as MMIO addresses are taken from Host DT and 
not all HW is virtualization friendly
(have HW modules at least 4K page aligned). What is not expected - is to get 
access to one additional page.


> > Note. paddr_to_pfn_aligned() has PAGE_ALIGN() inside.
> >
> > Fix it, by using paddr_to_pfn(addr + len - 1) in both places to get correct
> > end address of the iomem_range.
>  > > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>
> I think this wants to have a fixes tag and possibly split in two because I 
> suspect we may need to backport the changes to different versions.
Ok.

For the second chunk it seems obvious:
Fixes: 57d4d7d4e8f3b (arm/asm/setup.h: Update struct map_range_data to add 
rangeset.")

For the first one - not sure, seems:
Fixes: 33233c2758345 ("arch/arm: domain build: let dom0 access I/O memory of mapped 
devices")

> > ---
> >
> > Hi All,
> >
> > I have a question - the paddr_to_pfn_aligned() is not used any more,
> > should I remove it as part of this patch?
> I would keep it. It might be used in the future.
[...]

Thank you for review.
BR,
-grygorii