| Message ID | 20250214125505.2862809-1-grygorii_strashko@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: fix iomem_ranges cfg in map_range_to_domain() | expand |
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". > > 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. 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? > > 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@epam.com> 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. > --- > > 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. > > xen/arch/arm/device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c > index 5610cddcba8e..5e1c1cc326ac 100644 > --- a/xen/arch/arm/device.c > +++ b/xen/arch/arm/device.c > @@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev, > strlen("/reserved-memory/")) != 0 ) > { > res = iomem_permit_access(d, paddr_to_pfn(addr), > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > + paddr_to_pfn(addr + len - 1)); > if ( res ) > { > printk(XENLOG_ERR "Unable to permit to dom%d access to" > @@ -107,7 +107,7 @@ int map_range_to_domain(const struct dt_device_node *dev, > { > res = rangeset_add_range(mr_data->iomem_ranges, > paddr_to_pfn(addr), > - paddr_to_pfn_aligned(addr + len - 1)); > + paddr_to_pfn(addr + len - 1)); > if ( res ) > return res; > } Cheers,
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@epam.com> > > 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
On 14/02/2025 2:10 pm, Grygorii Strashko wrote: >>> >>> 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"? I agree Xen needs to be better (and by that, I mean consistent and clear), but there are subtle bugs with most approaches like this. Any exclusive bound, as well as counts like this need $n+1 bits of arithmetic when you want to describe the boundaries of the space. There is also a boundary condition for counts. What map_foo(x, 0) mean? Real hardware uses "last" for describing boundaries (x86 specifically calls this "limit" in the ISA, but it's a trick used by other architectures too). Unlike "end", it's clearly an inclusive bound. Personally, I'd like to see Xen switch to "start, last" pairs. It's unambiguous and has fewest boundary cases. ~Andrew
Hi Andrew, Julien, On 14.02.25 16:25, Andrew Cooper wrote: > On 14/02/2025 2:10 pm, Grygorii Strashko wrote: >>>> >>>> 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"? > > I agree Xen needs to be better (and by that, I mean consistent and > clear), but there are subtle bugs with most approaches like this. > > Any exclusive bound, as well as counts like this need $n+1 bits of > arithmetic when you want to describe the boundaries of the space. > > There is also a boundary condition for counts. What map_foo(x, 0) mean? > > Real hardware uses "last" for describing boundaries (x86 specifically > calls this "limit" in the ISA, but it's a trick used by other > architectures too). Unlike "end", it's clearly an inclusive bound. > > Personally, I'd like to see Xen switch to "start, last" pairs. It's > unambiguous and has fewest boundary cases. I'm preparing for re-sending the changes with applied Julien's comments, but I'd like to ask for clarification, as I'm not fully understand if I need to perform any other actions regarding above comment, or not. Sorry. BR, -grygorii
Hi Andrew, On 14/02/2025 14:25, Andrew Cooper wrote: > On 14/02/2025 2:10 pm, Grygorii Strashko wrote: >>>> >>>> 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"? > > I agree Xen needs to be better (and by that, I mean consistent and > clear), but there are subtle bugs with most approaches like this. > > Any exclusive bound, as well as counts like this need $n+1 bits of > arithmetic when you want to describe the boundaries of the space. > > There is also a boundary condition for counts. What map_foo(x, 0) mean? I would consider "0" has invalid. > > Real hardware uses "last" for describing boundaries (x86 specifically > calls this "limit" in the ISA, but it's a trick used by other > architectures too). Unlike "end", it's clearly an inclusive bound. > > Personally, I'd like to see Xen switch to "start, last" pairs. It's > unambiguous and has fewest boundary cases. "start, last" would also work for me. Cheers,
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index 5610cddcba8e..5e1c1cc326ac 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev, strlen("/reserved-memory/")) != 0 ) { res = iomem_permit_access(d, paddr_to_pfn(addr), - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); + paddr_to_pfn(addr + len - 1)); if ( res ) { printk(XENLOG_ERR "Unable to permit to dom%d access to" @@ -107,7 +107,7 @@ int map_range_to_domain(const struct dt_device_node *dev, { res = rangeset_add_range(mr_data->iomem_ranges, paddr_to_pfn(addr), - paddr_to_pfn_aligned(addr + len - 1)); + paddr_to_pfn(addr + len - 1)); if ( res ) return res; }
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. For example, requested range: 00e6140000 - 00e6141004 should set e6140:e6141 nr=2, but will configure e6140 e6142 nr=3 instead. 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@epam.com> --- 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? xen/arch/arm/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)