| Message ID | 20170908144849.2958-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/08/2017 10:48 AM, Juergen Gross wrote: > Based on the maximum page number of the host select either grant v1 or > grant v2. > > For testing purposes add a way to specify the grant interface version > via a boot parameter. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
On 08/09/17 15:48, Juergen Gross wrote: > static void gnttab_request_version(void) > { > - int rc; > + long rc; > struct gnttab_set_version gsv; > > - gsv.version = 1; > + rc = HYPERVISOR_memory_op(XENMEM_maximum_ram_page, NULL); This hypercall is information leak and layering violation. Please can we avoid adding more dependence on its presence? (I'm got a proto-series which strips various corners off the hypervisor for attack surface reduction purposes, and this hypercall is one victim which is restricted to privileged domains only.) For translated guests, it is definitely not the right number to check. What matters is the maximum frame inside the translated guest, not on the host. For PV guests, I'm not sure what to suggest, but the result of XENMEM_maximum_ram_page isn't applicable. Xen's max_page can increase at runtime through memory hotplug, after which ballooning operations can leave Linux with a frame it wishes to grant which exceeds the limit calculated here. The more I look into this, the more of a mess it turns out to be. ~Andrew > + if (rc < 0 || !(rc >> 32)) > + gsv.version = 1; > + else > + gsv.version = 2; > + if (xen_gnttab_version >= 1 && xen_gnttab_version <= 2) > + gsv.version = xen_gnttab_version; > > rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1); > if (rc == 0 && gsv.version == 2)
On 12/09/17 20:54, Andrew Cooper wrote: > On 08/09/17 15:48, Juergen Gross wrote: >> static void gnttab_request_version(void) >> { >> - int rc; >> + long rc; >> struct gnttab_set_version gsv; >> >> - gsv.version = 1; >> + rc = HYPERVISOR_memory_op(XENMEM_maximum_ram_page, NULL); > > This hypercall is information leak and layering violation. Please can > we avoid adding more dependence on its presence? (I'm got a > proto-series which strips various corners off the hypervisor for attack > surface reduction purposes, and this hypercall is one victim which is > restricted to privileged domains only.) > > For translated guests, it is definitely not the right number to check. > What matters is the maximum frame inside the translated guest, not on > the host. Oh, right. > For PV guests, I'm not sure what to suggest, but the result of > XENMEM_maximum_ram_page isn't applicable. Xen's max_page can increase > at runtime through memory hotplug, after which ballooning operations can > leave Linux with a frame it wishes to grant which exceeds the limit > calculated here. We need a way to decide whether V2 is to be selected. Is there a way to determine which is the highest physical address being available for memory hotplug on a system? Something in ACPI tables perhaps? Juergen
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c index 573af785c425..c8479cb4c0dc 100644 --- a/drivers/xen/grant-table.c +++ b/drivers/xen/grant-table.c @@ -43,6 +43,7 @@ #include <linux/hardirq.h> #include <linux/workqueue.h> #include <linux/ratelimit.h> +#include <linux/moduleparam.h> #include <xen/xen.h> #include <xen/interface/xen.h> @@ -68,6 +69,8 @@ static int gnttab_free_count; static grant_ref_t gnttab_free_head; static DEFINE_SPINLOCK(gnttab_list_lock); struct grant_frames xen_auto_xlat_grant_frames; +static unsigned int xen_gnttab_version; +module_param_named(version, xen_gnttab_version, uint, 0); static union { struct grant_entry_v1 *v1; @@ -1191,10 +1194,16 @@ static const struct gnttab_ops gnttab_v2_ops = { static void gnttab_request_version(void) { - int rc; + long rc; struct gnttab_set_version gsv; - gsv.version = 1; + rc = HYPERVISOR_memory_op(XENMEM_maximum_ram_page, NULL); + if (rc < 0 || !(rc >> 32)) + gsv.version = 1; + else + gsv.version = 2; + if (xen_gnttab_version >= 1 && xen_gnttab_version <= 2) + gsv.version = xen_gnttab_version; rc = HYPERVISOR_grant_table_op(GNTTABOP_set_version, &gsv, 1); if (rc == 0 && gsv.version == 2)
Based on the maximum page number of the host select either grant v1 or grant v2. For testing purposes add a way to specify the grant interface version via a boot parameter. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/grant-table.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)