* [PATCH v2 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384
@ 2021-12-10 9:38 Jan Beulich
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jan Beulich @ 2021-12-10 9:38 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
The original 1st patch has meanwhile gone in, but the adjustment requested
for patch 2 required a new prereq patch.
1: mm: introduce INVALID_{G,M}FN_RAW
2: memory: XENMEM_add_to_physmap (almost) wrapping checks
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW
2021-12-10 9:38 [PATCH v2 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich
@ 2021-12-10 9:39 ` Jan Beulich
2021-12-10 9:51 ` Oleksandr Andrushchenko
2021-12-17 14:46 ` Julien Grall
2021-12-10 9:40 ` [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich
2022-01-27 14:53 ` [PATCH v3] " Jan Beulich
2 siblings, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2021-12-10 9:39 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
This allows properly tying together INVALID_{G,M}FN and
INVALID_{G,M}FN_INITIALIZER as well as using the actual values in
compile time constant expressions (or even preprocessor dirctives).
Since INVALID_PFN is unused, and with x86'es paging_mark_pfn_dirty()
being the only user of pfn_t it also doesn't seem likely that new uses
would appear, remove that one at this same occasion.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/include/xen/mm-frame.h
+++ b/xen/include/xen/mm-frame.h
@@ -6,12 +6,13 @@
TYPE_SAFE(unsigned long, mfn);
#define PRI_mfn "05lx"
-#define INVALID_MFN _mfn(~0UL)
+#define INVALID_MFN_RAW (~0UL)
+#define INVALID_MFN _mfn(INVALID_MFN_RAW)
/*
* To be used for global variable initialization. This workaround a bug
* in GCC < 5.0.
*/
-#define INVALID_MFN_INITIALIZER { ~0UL }
+#define INVALID_MFN_INITIALIZER { INVALID_MFN_RAW }
#ifndef mfn_t
#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
@@ -44,12 +45,13 @@ static inline bool_t mfn_eq(mfn_t x, mfn
TYPE_SAFE(unsigned long, gfn);
#define PRI_gfn "05lx"
-#define INVALID_GFN _gfn(~0UL)
+#define INVALID_GFN_RAW (~0UL)
+#define INVALID_GFN _gfn(INVALID_GFN_RAW)
/*
* To be used for global variable initialization. This workaround a bug
* in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
*/
-#define INVALID_GFN_INITIALIZER { ~0UL }
+#define INVALID_GFN_INITIALIZER { INVALID_GFN_RAW }
#ifndef gfn_t
#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
@@ -82,7 +84,6 @@ static inline bool_t gfn_eq(gfn_t x, gfn
TYPE_SAFE(unsigned long, pfn);
#define PRI_pfn "05lx"
-#define INVALID_PFN (~0UL)
#ifndef pfn_t
#define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks
2021-12-10 9:38 [PATCH v2 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
@ 2021-12-10 9:40 ` Jan Beulich
2021-12-10 14:10 ` Oleksandr Andrushchenko
2022-01-27 14:53 ` [PATCH v3] " Jan Beulich
2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-12-10 9:40 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Determining that behavior is correct (i.e. results in failure) for a
passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a bit
more obvious by checking input in generic code - both for singular
requests to not match the value and for range ones to not pass / wrap
through it.
For Arm similarly make more obvious that no wrapping of MFNs passed
for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
Drop the "nr" parameter of the function to avoid future callers
appearing which might not themselves check for wrapping. Otherwise
the respective ASSERT() in rangeset_contains_range() could trigger.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add comment to BUILD_BUG_ON(). Avoid transiently #define-ing _gfn()
(by way of new prereq patch).
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
break;
}
case XENMAPSPACE_dev_mmio:
- rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
+ rc = map_dev_mmio_region(d, gfn, _mfn(idx));
return rc;
default:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1357,19 +1357,18 @@ int unmap_mmio_regions(struct domain *d,
int map_dev_mmio_region(struct domain *d,
gfn_t gfn,
- unsigned long nr,
mfn_t mfn)
{
int res;
- if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
+ if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
return 0;
- res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
+ res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
if ( res < 0 )
{
- printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n",
- mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
+ printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
+ mfn_x(mfn), d);
return res;
}
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4150,7 +4150,10 @@ int gnttab_map_frame(struct domain *d, u
bool status = false;
if ( gfn_eq(gfn, INVALID_GFN) )
+ {
+ ASSERT_UNREACHABLE();
return -EINVAL;
+ }
grant_write_lock(gt);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -832,6 +832,9 @@ int xenmem_add_to_physmap(struct domain
return -EACCES;
}
+ if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
+ return -EINVAL;
+
if ( xatp->space == XENMAPSPACE_gmfn_foreign )
extra.foreign_domid = DOMID_INVALID;
@@ -842,6 +845,18 @@ int xenmem_add_to_physmap(struct domain
if ( xatp->size < start )
return -EILSEQ;
+ if ( xatp->gpfn + xatp->size < xatp->gpfn ||
+ xatp->idx + xatp->size < xatp->idx )
+ {
+ /*
+ * Make sure INVALID_GFN is the highest representable value, i.e.
+ * guaranteeing that it won't fall in the middle of the
+ * [xatp->gpfn, xatp->gpfn + xatp->size) range checked above.
+ */
+ BUILD_BUG_ON(INVALID_GFN_RAW + 1);
+ return -EOVERFLOW;
+ }
+
xatp->idx += start;
xatp->gpfn += start;
xatp->size -= start;
@@ -962,6 +977,9 @@ static int xenmem_add_to_physmap_batch(s
extent, 1)) )
return -EFAULT;
+ if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
+ return -EINVAL;
+
rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
idx, _gfn(gpfn));
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d,
int map_dev_mmio_region(struct domain *d,
gfn_t gfn,
- unsigned long nr,
mfn_t mfn);
int p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
@ 2021-12-10 9:51 ` Oleksandr Andrushchenko
2021-12-17 14:46 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Oleksandr Andrushchenko @ 2021-12-10 9:51 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Hi, Jan!
On 10.12.21 11:39, Jan Beulich wrote:
> This allows properly tying together INVALID_{G,M}FN and
> INVALID_{G,M}FN_INITIALIZER as well as using the actual values in
> compile time constant expressions (or even preprocessor dirctives).
s/dirctives/directives
>
> Since INVALID_PFN is unused, and with x86'es paging_mark_pfn_dirty()
> being the only user of pfn_t it also doesn't seem likely that new uses
> would appear, remove that one at this same occasion.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
With the above nit fixed:
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> v2: New.
>
> --- a/xen/include/xen/mm-frame.h
> +++ b/xen/include/xen/mm-frame.h
> @@ -6,12 +6,13 @@
>
> TYPE_SAFE(unsigned long, mfn);
> #define PRI_mfn "05lx"
> -#define INVALID_MFN _mfn(~0UL)
> +#define INVALID_MFN_RAW (~0UL)
> +#define INVALID_MFN _mfn(INVALID_MFN_RAW)
> /*
> * To be used for global variable initialization. This workaround a bug
> * in GCC < 5.0.
> */
> -#define INVALID_MFN_INITIALIZER { ~0UL }
> +#define INVALID_MFN_INITIALIZER { INVALID_MFN_RAW }
>
> #ifndef mfn_t
> #define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
> @@ -44,12 +45,13 @@ static inline bool_t mfn_eq(mfn_t x, mfn
>
> TYPE_SAFE(unsigned long, gfn);
> #define PRI_gfn "05lx"
> -#define INVALID_GFN _gfn(~0UL)
> +#define INVALID_GFN_RAW (~0UL)
> +#define INVALID_GFN _gfn(INVALID_GFN_RAW)
> /*
> * To be used for global variable initialization. This workaround a bug
> * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
> */
> -#define INVALID_GFN_INITIALIZER { ~0UL }
> +#define INVALID_GFN_INITIALIZER { INVALID_GFN_RAW }
>
> #ifndef gfn_t
> #define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
> @@ -82,7 +84,6 @@ static inline bool_t gfn_eq(gfn_t x, gfn
>
> TYPE_SAFE(unsigned long, pfn);
> #define PRI_pfn "05lx"
> -#define INVALID_PFN (~0UL)
>
> #ifndef pfn_t
> #define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks
2021-12-10 9:40 ` [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich
@ 2021-12-10 14:10 ` Oleksandr Andrushchenko
2021-12-13 7:28 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Oleksandr Andrushchenko @ 2021-12-10 14:10 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Hi, Jan!
On 10.12.21 11:40, Jan Beulich wrote:
> Determining that behavior is correct (i.e. results in failure) for a
> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a bit
> more obvious by checking input in generic code - both for singular
> requests to not match the value and for range ones to not pass / wrap
> through it.
>
> For Arm similarly make more obvious that no wrapping of MFNs passed
> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
> Drop the "nr" parameter of the function to avoid future callers
> appearing which might not themselves check for wrapping. Otherwise
> the respective ASSERT() in rangeset_contains_range() could trigger.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add comment to BUILD_BUG_ON(). Avoid transiently #define-ing _gfn()
> (by way of new prereq patch).
>
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
> break;
> }
> case XENMAPSPACE_dev_mmio:
> - rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
> + rc = map_dev_mmio_region(d, gfn, _mfn(idx));
Technically this is ok, but reads odd now: the function maps a single
page, but its name has "region" in it (which might also be ok, e.g.
for a region of a single page).
I think it is worth either implementing full mfn range check inside
map_dev_mmio_region or renaming it to something else:
with mfn check map_dev_mmio_region will indeed be able to map
a region consisting of multiple pages and perform required validation.
Thank you,
Oleksandr
> return rc;
>
> default:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1357,19 +1357,18 @@ int unmap_mmio_regions(struct domain *d,
>
> int map_dev_mmio_region(struct domain *d,
> gfn_t gfn,
> - unsigned long nr,
> mfn_t mfn)
> {
> int res;
>
> - if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
> + if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
> return 0;
>
> - res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
> + res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
> if ( res < 0 )
> {
> - printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n",
> - mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
> + printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
> + mfn_x(mfn), d);
> return res;
> }
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4150,7 +4150,10 @@ int gnttab_map_frame(struct domain *d, u
> bool status = false;
>
> if ( gfn_eq(gfn, INVALID_GFN) )
> + {
> + ASSERT_UNREACHABLE();
> return -EINVAL;
> + }
>
> grant_write_lock(gt);
>
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -832,6 +832,9 @@ int xenmem_add_to_physmap(struct domain
> return -EACCES;
> }
>
> + if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
> + return -EINVAL;
> +
> if ( xatp->space == XENMAPSPACE_gmfn_foreign )
> extra.foreign_domid = DOMID_INVALID;
>
> @@ -842,6 +845,18 @@ int xenmem_add_to_physmap(struct domain
> if ( xatp->size < start )
> return -EILSEQ;
>
> + if ( xatp->gpfn + xatp->size < xatp->gpfn ||
> + xatp->idx + xatp->size < xatp->idx )
> + {
> + /*
> + * Make sure INVALID_GFN is the highest representable value, i.e.
> + * guaranteeing that it won't fall in the middle of the
> + * [xatp->gpfn, xatp->gpfn + xatp->size) range checked above.
> + */
> + BUILD_BUG_ON(INVALID_GFN_RAW + 1);
> + return -EOVERFLOW;
> + }
> +
> xatp->idx += start;
> xatp->gpfn += start;
> xatp->size -= start;
> @@ -962,6 +977,9 @@ static int xenmem_add_to_physmap_batch(s
> extent, 1)) )
> return -EFAULT;
>
> + if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
> + return -EINVAL;
> +
> rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
> idx, _gfn(gpfn));
>
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d,
>
> int map_dev_mmio_region(struct domain *d,
> gfn_t gfn,
> - unsigned long nr,
> mfn_t mfn);
>
> int p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr,
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks
2021-12-10 14:10 ` Oleksandr Andrushchenko
@ 2021-12-13 7:28 ` Jan Beulich
2021-12-13 18:41 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2021-12-13 7:28 UTC (permalink / raw)
To: Oleksandr Andrushchenko, Julien Grall, Stefano Stabellini
Cc: Andrew Cooper, George Dunlap, Wei Liu, xen-devel
On 10.12.2021 15:10, Oleksandr Andrushchenko wrote:
> On 10.12.21 11:40, Jan Beulich wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
>> break;
>> }
>> case XENMAPSPACE_dev_mmio:
>> - rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
>> + rc = map_dev_mmio_region(d, gfn, _mfn(idx));
> Technically this is ok, but reads odd now: the function maps a single
> page, but its name has "region" in it (which might also be ok, e.g.
> for a region of a single page).
>
> I think it is worth either implementing full mfn range check inside
> map_dev_mmio_region or renaming it to something else:
> with mfn check map_dev_mmio_region will indeed be able to map
> a region consisting of multiple pages and perform required validation.
Well, I had no maintainer comments on v1 regarding the name. I'd be
happy to rename to e.g. map_dev_mmio_page(), so long as there can be
an agreed upon name before I submit a possible v3. Julien, Stefano?
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks
2021-12-13 7:28 ` Jan Beulich
@ 2021-12-13 18:41 ` Stefano Stabellini
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Stabellini @ 2021-12-13 18:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Oleksandr Andrushchenko, Julien Grall, Stefano Stabellini,
Andrew Cooper, George Dunlap, Wei Liu, xen-devel
On Mon, 13 Dec 2021, Jan Beulich wrote:
> On 10.12.2021 15:10, Oleksandr Andrushchenko wrote:
> > On 10.12.21 11:40, Jan Beulich wrote:
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
> >> break;
> >> }
> >> case XENMAPSPACE_dev_mmio:
> >> - rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
> >> + rc = map_dev_mmio_region(d, gfn, _mfn(idx));
> > Technically this is ok, but reads odd now: the function maps a single
> > page, but its name has "region" in it (which might also be ok, e.g.
> > for a region of a single page).
> >
> > I think it is worth either implementing full mfn range check inside
> > map_dev_mmio_region or renaming it to something else:
> > with mfn check map_dev_mmio_region will indeed be able to map
> > a region consisting of multiple pages and perform required validation.
>
> Well, I had no maintainer comments on v1 regarding the name. I'd be
> happy to rename to e.g. map_dev_mmio_page(), so long as there can be
> an agreed upon name before I submit a possible v3. Julien, Stefano?
I like map_dev_mmio_page
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
2021-12-10 9:51 ` Oleksandr Andrushchenko
@ 2021-12-17 14:46 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Julien Grall @ 2021-12-17 14:46 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu
Hi Jan,
On 10/12/2021 09:39, Jan Beulich wrote:
> This allows properly tying together INVALID_{G,M}FN and
> INVALID_{G,M}FN_INITIALIZER as well as using the actual values in
> compile time constant expressions (or even preprocessor dirctives).
>
> Since INVALID_PFN is unused, and with x86'es paging_mark_pfn_dirty()
> being the only user of pfn_t it also doesn't seem likely that new uses
> would appear, remove that one at this same occasion.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] memory: XENMEM_add_to_physmap (almost) wrapping checks
2021-12-10 9:38 [PATCH v2 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
2021-12-10 9:40 ` [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich
@ 2022-01-27 14:53 ` Jan Beulich
2022-01-31 18:30 ` Julien Grall
2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-01-27 14:53 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Determining that behavior is correct (i.e. results in failure) for a
passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a bit
more obvious by checking input in generic code - both for singular
requests to not match the value and for range ones to not pass / wrap
through it.
For Arm similarly make more obvious that no wrapping of MFNs passed
for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
Drop the "nr" parameter of the function to avoid future callers
appearing which might not themselves check for wrapping. Otherwise
the respective ASSERT() in rangeset_contains_range() could trigger.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Rename function to map_dev_mmio_page().
v2: Add comment to BUILD_BUG_ON(). Avoid transiently #define-ing _gfn()
(by way of new prereq patch).
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
break;
}
case XENMAPSPACE_dev_mmio:
- rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
+ rc = map_dev_mmio_page(d, gfn, _mfn(idx));
return rc;
default:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1355,21 +1355,18 @@ int unmap_mmio_regions(struct domain *d,
return p2m_remove_mapping(d, start_gfn, nr, mfn);
}
-int map_dev_mmio_region(struct domain *d,
- gfn_t gfn,
- unsigned long nr,
- mfn_t mfn)
+int map_dev_mmio_page(struct domain *d, gfn_t gfn, mfn_t mfn)
{
int res;
- if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
+ if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
return 0;
- res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
+ res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
if ( res < 0 )
{
- printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n",
- mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
+ printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
+ mfn_x(mfn), d);
return res;
}
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4157,7 +4157,10 @@ int gnttab_map_frame(struct domain *d, u
bool status = false;
if ( gfn_eq(gfn, INVALID_GFN) )
+ {
+ ASSERT_UNREACHABLE();
return -EINVAL;
+ }
grant_write_lock(gt);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -832,6 +832,9 @@ int xenmem_add_to_physmap(struct domain
return -EACCES;
}
+ if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
+ return -EINVAL;
+
if ( xatp->space == XENMAPSPACE_gmfn_foreign )
extra.foreign_domid = DOMID_INVALID;
@@ -842,6 +845,18 @@ int xenmem_add_to_physmap(struct domain
if ( xatp->size < start )
return -EILSEQ;
+ if ( xatp->gpfn + xatp->size < xatp->gpfn ||
+ xatp->idx + xatp->size < xatp->idx )
+ {
+ /*
+ * Make sure INVALID_GFN is the highest representable value, i.e.
+ * guaranteeing that it won't fall in the middle of the
+ * [xatp->gpfn, xatp->gpfn + xatp->size) range checked above.
+ */
+ BUILD_BUG_ON(INVALID_GFN_RAW + 1);
+ return -EOVERFLOW;
+ }
+
xatp->idx += start;
xatp->gpfn += start;
xatp->size -= start;
@@ -962,6 +977,9 @@ static int xenmem_add_to_physmap_batch(s
extent, 1)) )
return -EFAULT;
+ if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
+ return -EINVAL;
+
rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
idx, _gfn(gpfn));
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -295,10 +295,7 @@ int unmap_regions_p2mt(struct domain *d,
unsigned long nr,
mfn_t mfn);
-int map_dev_mmio_region(struct domain *d,
- gfn_t gfn,
- unsigned long nr,
- mfn_t mfn);
+int map_dev_mmio_page(struct domain *d, gfn_t gfn, mfn_t mfn);
int p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr,
mfn_t mfn, p2m_type_t t);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] memory: XENMEM_add_to_physmap (almost) wrapping checks
2022-01-27 14:53 ` [PATCH v3] " Jan Beulich
@ 2022-01-31 18:30 ` Julien Grall
0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2022-01-31 18:30 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu
Hi,
On 27/01/2022 14:53, Jan Beulich wrote:
> Determining that behavior is correct (i.e. results in failure) for a
> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a bit
> more obvious by checking input in generic code - both for singular
> requests to not match the value and for range ones to not pass / wrap
> through it.
>
> For Arm similarly make more obvious that no wrapping of MFNs passed
> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
> Drop the "nr" parameter of the function to avoid future callers
> appearing which might not themselves check for wrapping. Otherwise
> the respective ASSERT() in rangeset_contains_range() could trigger.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-31 18:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-10 9:38 [PATCH v2 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich
2021-12-10 9:39 ` [PATCH v2 1/2] mm: introduce INVALID_{G,M}FN_RAW Jan Beulich
2021-12-10 9:51 ` Oleksandr Andrushchenko
2021-12-17 14:46 ` Julien Grall
2021-12-10 9:40 ` [PATCH v2 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich
2021-12-10 14:10 ` Oleksandr Andrushchenko
2021-12-13 7:28 ` Jan Beulich
2021-12-13 18:41 ` Stefano Stabellini
2022-01-27 14:53 ` [PATCH v3] " Jan Beulich
2022-01-31 18:30 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.