Date: Tue, 20 Jun 2017 13:04:24 +0000 (UTC) From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <royger@FreeBSD.org> To: ports-committers@freebsd.org, svn-ports-all@freebsd.org, svn-ports-head@freebsd.org Subject: svn commit: r443949 - in head/emulators/xen-kernel: . files Message-ID: <201706201304.v5KD4OsQ035765@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: royger (src committer) Date: Tue Jun 20 13:04:23 2017 New Revision: 443949 URL: https://svnweb.freebsd.org/changeset/ports/443949 Log: xen: apply XSA-{217,218,219,220,221,222,224} Approved by: bapt Sponsored by: Citrix Systems R&D MFH: 2017Q2 Added: head/emulators/xen-kernel/files/0001-IOMMU-handle-IOMMU-mapping-and-unmapping-failures.patch (contents, props changed) head/emulators/xen-kernel/files/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch (contents, props changed) head/emulators/xen-kernel/files/0002-gnttab-fix-unmap-pin-accounting-race.patch (contents, props changed) head/emulators/xen-kernel/files/0002-gnttab-never-create-host-mapping-unless-asked-to.patch (contents, props changed) head/emulators/xen-kernel/files/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch (contents, props changed) head/emulators/xen-kernel/files/0003-gnttab-correct-logic-to-get-page-references-during-m.patch (contents, props changed) head/emulators/xen-kernel/files/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch (contents, props changed) head/emulators/xen-kernel/files/0004-gnttab-correct-maptrack-table-accesses.patch (contents, props changed) head/emulators/xen-kernel/files/xsa217.patch (contents, props changed) head/emulators/xen-kernel/files/xsa219-4.8.patch (contents, props changed) head/emulators/xen-kernel/files/xsa220-4.7.patch (contents, props changed) head/emulators/xen-kernel/files/xsa221.patch (contents, props changed) head/emulators/xen-kernel/files/xsa222-1-4.7.patch (contents, props changed) head/emulators/xen-kernel/files/xsa222-2-4.7.patch (contents, props changed) Modified: head/emulators/xen-kernel/Makefile Modified: head/emulators/xen-kernel/Makefile ============================================================================== --- head/emulators/xen-kernel/Makefile Tue Jun 20 10:52:53 2017 (r443948) +++ head/emulators/xen-kernel/Makefile Tue Jun 20 13:04:23 2017 (r443949) @@ -2,7 +2,7 @@ PORTNAME= xen PORTVERSION= 4.7.2 -PORTREVISION= 2 +PORTREVISION= 3 CATEGORIES= emulators MASTER_SITES= http://downloads.xenproject.org/release/xen/${PORTVERSION}/ PKGNAMESUFFIX= -kernel @@ -45,7 +45,21 @@ EXTRA_PATCHES= ${FILESDIR}/0001-xen-logdirty-prevent-p ${FILESDIR}/xsa212.patch:-p1 \ ${FILESDIR}/xsa213-4.7.patch:-p1 \ ${FILESDIR}/xsa214.patch:-p1 \ - ${FILESDIR}/xsa215.patch:-p1 + ${FILESDIR}/xsa215.patch:-p1 \ + ${FILESDIR}/xsa217.patch:-p1 \ + ${FILESDIR}/0001-IOMMU-handle-IOMMU-mapping-and-unmapping-failures.patch:-p1 \ + ${FILESDIR}/0002-gnttab-fix-unmap-pin-accounting-race.patch:-p1 \ + ${FILESDIR}/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch:-p1 \ + ${FILESDIR}/0004-gnttab-correct-maptrack-table-accesses.patch:-p1 \ + ${FILESDIR}/xsa219-4.8.patch:-p1 \ + ${FILESDIR}/xsa220-4.7.patch:-p1 \ + ${FILESDIR}/xsa221.patch:-p1 \ + ${FILESDIR}/xsa222-1-4.7.patch:-p1 \ + ${FILESDIR}/xsa222-2-4.7.patch:-p1 \ + ${FILESDIR}/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch:-p1 \ + ${FILESDIR}/0002-gnttab-never-create-host-mapping-unless-asked-to.patch:-p1 \ + ${FILESDIR}/0003-gnttab-correct-logic-to-get-page-references-during-m.patch:-p1 \ + ${FILESDIR}/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch:-p1 .include <bsd.port.options.mk> Added: head/emulators/xen-kernel/files/0001-IOMMU-handle-IOMMU-mapping-and-unmapping-failures.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0001-IOMMU-handle-IOMMU-mapping-and-unmapping-failures.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,78 @@ +From 03f872b98f24e25cafb478b5d7c34e1eb18e1e4c Mon Sep 17 00:00:00 2001 +From: Quan Xu <quan.xu@intel.com> +Date: Fri, 2 Jun 2017 12:30:34 +0100 +Subject: [PATCH 1/4] IOMMU: handle IOMMU mapping and unmapping failures + +Treat IOMMU mapping and unmapping failures as a fatal to the DomU +If IOMMU mapping and unmapping failed, crash the DomU and propagate +the error up to the call trees. + +No spamming of the log can occur. For DomU, we avoid logging any +message for already dying domains. For Dom0, that'll still be more +verbose than we'd really like, but it at least wouldn't outright +flood the console. + +Signed-off-by: Quan Xu <quan.xu@intel.com> +Reviewed-by: Kevin Tian <kevin.tian@intel.com> +Reviewed-by: Jan Beulich <jbeulich@suse.com> +--- + xen/drivers/passthrough/iommu.c | 30 ++++++++++++++++++++++++++++-- + 1 file changed, 28 insertions(+), 2 deletions(-) + +diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c +index 1a315ee..927966f 100644 +--- a/xen/drivers/passthrough/iommu.c ++++ b/xen/drivers/passthrough/iommu.c +@@ -239,21 +239,47 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, + unsigned int flags) + { + const struct domain_iommu *hd = dom_iommu(d); ++ int rc; + + if ( !iommu_enabled || !hd->platform_ops ) + return 0; + +- return hd->platform_ops->map_page(d, gfn, mfn, flags); ++ rc = hd->platform_ops->map_page(d, gfn, mfn, flags); ++ if ( unlikely(rc) ) ++ { ++ if ( !d->is_shutting_down && printk_ratelimit() ) ++ printk(XENLOG_ERR ++ "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n", ++ d->domain_id, gfn, mfn, rc); ++ ++ if ( !is_hardware_domain(d) ) ++ domain_crash(d); ++ } ++ ++ return rc; + } + + int iommu_unmap_page(struct domain *d, unsigned long gfn) + { + const struct domain_iommu *hd = dom_iommu(d); ++ int rc; + + if ( !iommu_enabled || !hd->platform_ops ) + return 0; + +- return hd->platform_ops->unmap_page(d, gfn); ++ rc = hd->platform_ops->unmap_page(d, gfn); ++ if ( unlikely(rc) ) ++ { ++ if ( !d->is_shutting_down && printk_ratelimit() ) ++ printk(XENLOG_ERR ++ "d%d: IOMMU unmapping gfn %#lx failed: %d\n", ++ d->domain_id, gfn, rc); ++ ++ if ( !is_hardware_domain(d) ) ++ domain_crash(d); ++ } ++ ++ return rc; + } + + static void iommu_free_pagetables(unsigned long unused) +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,111 @@ +From fd97f5f5ba9375163c8d8771fe551bb4a6423b36 Mon Sep 17 00:00:00 2001 +From: George Dunlap <george.dunlap@citrix.com> +Date: Thu, 15 Jun 2017 16:24:02 +0100 +Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap + +If a grant has been mapped with the GNTTAB_device_map flag, calling +grant_unmap_ref() with dev_bus_addr set to zero should cause the +GNTTAB_device_map part of the mapping to be left alone. + +Unfortunately, at the moment, op->dev_bus_addr is implicitly checked +before clearing the map and adjusting the pin count, but only the bits +above 12; and it is not checked at all before dropping page +references. This means a guest can repeatedly make such a call to +cause the reference count to drop to zero, causing the page to be +freed and re-used, even though it's still mapped in its pagetables. + +To fix this, always check op->dev_bus_addr explicitly for being +non-zero, as well as op->flag & GNTMAP_device_map, before doing +operations on the device_map. + +While we're here, make the logic a bit cleaner: + +* Always initialize op->frame to zero and set it from act->frame, to reduce the +chance of untrusted input being used + +* Explicitly check the full dev_bus_addr against act->frame << + PAGE_SHIFT, rather than ignoring the lower 12 bits + +This is part of XSA-224. + +Reported-by: Jan Beulich <jbeulich@suse.com> +Signed-off-by: George Dunlap <george.dunlap@citrix.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 23 +++++++++++------------ + 1 file changed, 11 insertions(+), 12 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index c4d73af..69cbdb6 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -1089,8 +1089,6 @@ __gnttab_unmap_common( + ld = current->domain; + lgt = ld->grant_table; + +- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT); +- + if ( unlikely(op->handle >= lgt->maptrack_limit) ) + { + gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle); +@@ -1174,16 +1172,14 @@ __gnttab_unmap_common( + goto act_release_out; + } + +- if ( op->frame == 0 ) +- { +- op->frame = act->frame; +- } +- else ++ op->frame = act->frame; ++ ++ if ( op->dev_bus_addr ) + { +- if ( unlikely(op->frame != act->frame) ) ++ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) ) + PIN_FAIL(act_release_out, GNTST_general_error, +- "Bad frame number doesn't match gntref. (%lx != %lx)\n", +- op->frame, act->frame); ++ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n", ++ op->dev_bus_addr, pfn_to_paddr(act->frame)); + + map->flags &= ~GNTMAP_device_map; + } +@@ -1276,7 +1272,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + else + status = &status_entry(rgt, op->ref); + +- if ( unlikely(op->frame != act->frame) ) ++ if ( op->dev_bus_addr && ++ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) ) + { + /* + * Suggests that __gntab_unmap_common failed early and so +@@ -1287,7 +1284,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + + pg = mfn_to_page(op->frame); + +- if ( op->flags & GNTMAP_device_map ) ++ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) ) + { + if ( !is_iomem_page(act->frame) ) + { +@@ -1358,6 +1355,7 @@ __gnttab_unmap_grant_ref( + /* Intialise these in case common contains old state */ + common->new_addr = 0; + common->rd = NULL; ++ common->frame = 0; + + __gnttab_unmap_common(common); + op->status = common->status; +@@ -1422,6 +1420,7 @@ __gnttab_unmap_and_replace( + /* Intialise these in case common contains old state */ + common->dev_bus_addr = 0; + common->rd = NULL; ++ common->frame = 0; + + __gnttab_unmap_common(common); + op->status = common->status; +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0002-gnttab-fix-unmap-pin-accounting-race.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0002-gnttab-fix-unmap-pin-accounting-race.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,102 @@ +From 2c146b4f763f47180a0effb8d8045b0ebb93652c Mon Sep 17 00:00:00 2001 +From: Jan Beulich <jbeulich@suse.com> +Date: Fri, 2 Jun 2017 12:22:42 +0100 +Subject: [PATCH 2/4] gnttab: fix unmap pin accounting race + +Once all {writable} mappings of a grant entry have been unmapped, the +hypervisor informs the guest that the grant entry has been released by +clearing the _GTF_{reading,writing} usage flags in the guest's grant +table as appropriate. + +Unfortunately, at the moment, the code that updates the accounting +happens in a different critical section than the one which updates the +usage flags; this means that under the right circumstances, there may be +a window in time after the hypervisor reported the grant as being free +during which the grant referee still had access to the page. + +Move the grant accounting code into the same critical section as the +reporting code to make sure this kind of race can't happen. + +This is part of XSA-218. + +Reported-by: Jann Horn <jannh.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 32 +++++++++++++++++--------------- + 1 file changed, 17 insertions(+), 15 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index 8b22299..cfc483f 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -1150,15 +1150,8 @@ __gnttab_unmap_common( + PIN_FAIL(act_release_out, GNTST_general_error, + "Bad frame number doesn't match gntref. (%lx != %lx)\n", + op->frame, act->frame); +- if ( op->flags & GNTMAP_device_map ) +- { +- ASSERT(act->pin & (GNTPIN_devw_mask | GNTPIN_devr_mask)); +- op->map->flags &= ~GNTMAP_device_map; +- if ( op->flags & GNTMAP_readonly ) +- act->pin -= GNTPIN_devr_inc; +- else +- act->pin -= GNTPIN_devw_inc; +- } ++ ++ op->map->flags &= ~GNTMAP_device_map; + } + + if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) +@@ -1168,12 +1161,7 @@ __gnttab_unmap_common( + op->flags)) < 0 ) + goto act_release_out; + +- ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)); + op->map->flags &= ~GNTMAP_host_map; +- if ( op->flags & GNTMAP_readonly ) +- act->pin -= GNTPIN_hstr_inc; +- else +- act->pin -= GNTPIN_hstw_inc; + } + + act_release_out: +@@ -1266,6 +1254,12 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + else + put_page_and_type(pg); + } ++ ++ ASSERT(act->pin & (GNTPIN_devw_mask | GNTPIN_devr_mask)); ++ if ( op->flags & GNTMAP_readonly ) ++ act->pin -= GNTPIN_devr_inc; ++ else ++ act->pin -= GNTPIN_devw_inc; + } + + if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) +@@ -1274,7 +1268,9 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + { + /* + * Suggests that __gntab_unmap_common failed in +- * replace_grant_host_mapping() so nothing further to do ++ * replace_grant_host_mapping() or IOMMU handling, so nothing ++ * further to do (short of re-establishing the mapping in the ++ * latter case). + */ + goto act_release_out; + } +@@ -1285,6 +1281,12 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + put_page_type(pg); + put_page(pg); + } ++ ++ ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)); ++ if ( op->flags & GNTMAP_readonly ) ++ act->pin -= GNTPIN_hstr_inc; ++ else ++ act->pin -= GNTPIN_hstw_inc; + } + + if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0002-gnttab-never-create-host-mapping-unless-asked-to.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0002-gnttab-never-create-host-mapping-unless-asked-to.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,42 @@ +From 8894a0c20d920aada305aade0591c1e77167b1db Mon Sep 17 00:00:00 2001 +From: Jan Beulich <jbeulich@suse.com> +Date: Fri, 2 Jun 2017 15:21:27 +0100 +Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to + +We shouldn't create a host mapping unless asked to even in the case of +mapping a granted MMIO page. In particular the mapping wouldn't be torn +down when processing the matching unmap request. + +This is part of XSA-224. + +Reported-by: Jan Beulich <jbeulich@suse.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 11 +++++++---- + 1 file changed, 7 insertions(+), 4 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index 69cbdb6..452538e 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -911,10 +911,13 @@ __gnttab_map_grant_ref( + goto undo_out; + } + +- rc = create_grant_host_mapping( +- op->host_addr, frame, op->flags, cache_flags); +- if ( rc != GNTST_okay ) +- goto undo_out; ++ if ( op->flags & GNTMAP_host_map ) ++ { ++ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, ++ cache_flags); ++ if ( rc != GNTST_okay ) ++ goto undo_out; ++ } + } + else if ( owner == rd || owner == dom_cow ) + { +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,232 @@ +From 39b704785a8d330c02e8e2d2368c80dbaf679bc0 Mon Sep 17 00:00:00 2001 +From: George Dunlap <george.dunlap@citrix.com> +Date: Thu, 15 Jun 2017 12:05:14 +0100 +Subject: [PATCH 3/4] gnttab: Avoid potential double-put of maptrack entry + +Each grant mapping for a particular domain is tracked by an in-Xen +"maptrack" entry. This entry is is referenced by a "handle", which is +given to the guest when it calls gnttab_map_grant_ref(). + +There are two types of mapping a particular handle can refer to: +GNTMAP_host_map and GNTMAP_device_map. A given +gnttab_unmap_grant_ref() call can remove either only one or both of +these entries. When a particular handle has no entries left, it must +be freed. + +gnttab_unmap_grant_ref() loops through its grant unmap request list +twice. It first removes entries from any host pagetables and (if +appropraite) iommus; then it does a single domain TLB flush; then it +does the clean-up, including telling the granter that entries are no +longer being used (if appropriate). + +At the moment, it's during the first pass that the maptrack flags are +cleared, but the second pass that the maptrack entry is freed. + +Unfortunately this allows the following race, which results in a +double-free: + + A: (pass 1) clear host_map + B: (pass 1) clear device_map + A: (pass 2) See that maptrack entry has no mappings, free it + B: (pass 2) See that maptrack entry has no mappings, free it # + +Unfortunately, unlike the active entry pinning update, we can't simply +move the maptrack flag changes to the second half, because the +maptrack flags are used to determine if iommu entries need to be +added: a domain's iommu must never have fewer permissions than the +maptrack flags indicate, or a subsequent map_grant_ref() might fail to +add the necessary iommu entries. + +Instead, free the maptrack entry in the first pass if there are no +further mappings. + +This is part of XSA-218. + +Reported-by: Jan Beulich <jbeulich.com> +Signed-off-by: George Dunlap <george.dunlap@citrix.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 79 +++++++++++++++++++++++++++++++++--------------- + 1 file changed, 54 insertions(+), 25 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index cfc483f..81a1a8b 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -98,8 +98,8 @@ struct gnttab_unmap_common { + /* Shared state beteen *_unmap and *_unmap_complete */ + u16 flags; + unsigned long frame; +- struct grant_mapping *map; + struct domain *rd; ++ grant_ref_t ref; + }; + + /* Number of unmap operations that are done between each tlb flush */ +@@ -1079,6 +1079,8 @@ __gnttab_unmap_common( + struct grant_table *lgt, *rgt; + struct active_grant_entry *act; + s16 rc = 0; ++ struct grant_mapping *map; ++ bool_t put_handle = 0; + + ld = current->domain; + lgt = ld->grant_table; +@@ -1092,11 +1094,11 @@ __gnttab_unmap_common( + return; + } + +- op->map = &maptrack_entry(lgt, op->handle); ++ map = &maptrack_entry(lgt, op->handle); + + grant_read_lock(lgt); + +- if ( unlikely(!read_atomic(&op->map->flags)) ) ++ if ( unlikely(!read_atomic(&map->flags)) ) + { + grant_read_unlock(lgt); + gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle); +@@ -1104,7 +1106,7 @@ __gnttab_unmap_common( + return; + } + +- dom = op->map->domid; ++ dom = map->domid; + grant_read_unlock(lgt); + + if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) +@@ -1129,16 +1131,43 @@ __gnttab_unmap_common( + + grant_read_lock(rgt); + +- op->flags = read_atomic(&op->map->flags); +- if ( unlikely(!op->flags) || unlikely(op->map->domid != dom) ) ++ op->rd = rd; ++ op->ref = map->ref; ++ ++ /* ++ * We can't assume there was no racing unmap for this maptrack entry, ++ * and hence we can't assume map->ref is valid for rd. While the checks ++ * below (with the active entry lock held) will reject any such racing ++ * requests, we still need to make sure we don't attempt to acquire an ++ * invalid lock. ++ */ ++ smp_rmb(); ++ if ( unlikely(op->ref >= nr_grant_entries(rgt)) ) + { +- gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle); ++ gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); + rc = GNTST_bad_handle; +- goto unmap_out; ++ goto unlock_out; + } + +- op->rd = rd; +- act = active_entry_acquire(rgt, op->map->ref); ++ act = active_entry_acquire(rgt, op->ref); ++ ++ /* ++ * Note that we (ab)use the active entry lock here to protect against ++ * multiple unmaps of the same mapping here. We don't want to hold lgt's ++ * lock, and we only hold rgt's lock for reading (but the latter wouldn't ++ * be the right one anyway). Hence the easiest is to rely on a lock we ++ * hold anyway; see docs/misc/grant-tables.txt's "Locking" section. ++ */ ++ ++ op->flags = read_atomic(&map->flags); ++ smp_rmb(); ++ if ( unlikely(!op->flags) || unlikely(map->domid != dom) || ++ unlikely(map->ref != op->ref) ) ++ { ++ gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); ++ rc = GNTST_bad_handle; ++ goto act_release_out; ++ } + + if ( op->frame == 0 ) + { +@@ -1151,7 +1180,7 @@ __gnttab_unmap_common( + "Bad frame number doesn't match gntref. (%lx != %lx)\n", + op->frame, act->frame); + +- op->map->flags &= ~GNTMAP_device_map; ++ map->flags &= ~GNTMAP_device_map; + } + + if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) +@@ -1161,14 +1190,23 @@ __gnttab_unmap_common( + op->flags)) < 0 ) + goto act_release_out; + +- op->map->flags &= ~GNTMAP_host_map; ++ map->flags &= ~GNTMAP_host_map; ++ } ++ ++ if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ) ++ { ++ map->flags = 0; ++ put_handle = 1; + } + + act_release_out: + active_entry_release(act); +- unmap_out: ++ unlock_out: + grant_read_unlock(rgt); + ++ if ( put_handle ) ++ put_maptrack_handle(lgt, op->handle); ++ + if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) ) + { + unsigned int kind; +@@ -1205,7 +1243,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + grant_entry_header_t *sha; + struct page_info *pg; + uint16_t *status; +- bool_t put_handle = 0; + + if ( rd == NULL ) + { +@@ -1226,13 +1263,13 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + if ( rgt->gt_version == 0 ) + goto unlock_out; + +- act = active_entry_acquire(rgt, op->map->ref); +- sha = shared_entry_header(rgt, op->map->ref); ++ act = active_entry_acquire(rgt, op->ref); ++ sha = shared_entry_header(rgt, op->ref); + + if ( rgt->gt_version == 1 ) + status = &sha->flags; + else +- status = &status_entry(rgt, op->map->ref); ++ status = &status_entry(rgt, op->ref); + + if ( unlikely(op->frame != act->frame) ) + { +@@ -1289,9 +1326,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + act->pin -= GNTPIN_hstw_inc; + } + +- if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) +- put_handle = 1; +- + if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && + !(op->flags & GNTMAP_readonly) ) + gnttab_clear_flag(_GTF_writing, status); +@@ -1304,11 +1338,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + unlock_out: + grant_read_unlock(rgt); + +- if ( put_handle ) +- { +- op->map->flags = 0; +- put_maptrack_handle(ld->grant_table, op->handle); +- } + rcu_unlock_domain(rd); + } + +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0003-gnttab-correct-logic-to-get-page-references-during-m.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0003-gnttab-correct-logic-to-get-page-references-during-m.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,186 @@ +From 5d491e3cf32ff03552db9d66e842964fec06dcd4 Mon Sep 17 00:00:00 2001 +From: George Dunlap <george.dunlap@citrix.com> +Date: Fri, 2 Jun 2017 15:21:27 +0100 +Subject: [PATCH 3/4] gnttab: correct logic to get page references during map + requests + +The rules for reference counting are somewhat complicated: + +* Each of GNTTAB_host_map and GNTTAB_device_map need their own +reference count + +* If the mapping is writeable: + - GNTTAB_host_map needs a type count under only some conditions + - GNTTAB_device_map always needs a type count + +If the mapping succeeds, we need to keep all of these; if the mapping +fails, we need to release whatever references we have acquired so far. + +Additionally, the code that does a lot of this calculation "inherits" +a reference as part of the process of finding out who the owner is. + +Finally, if the grant is mapped as writeable (without the +GNTMAP_readonly flag), but the hypervisor cannot grab a +PGT_writeable_page type, the entire operation should fail. + +Unfortunately, the current code has several logic holes: + +* If a grant is mapped only GNTTAB_device_map, and with a writeable + mapping, but in conditions where a *host* type count is not + necessary, the code will fail to grab the necessary type count. + +* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map, + with a writeable mapping, in conditions where the host type count is + not necessary, *and* where the page cannot be changed to type + PGT_writeable, the condition will not be detected. + +In both cases, this means that on success, the type count will be +erroneously reduced when the grant is unmapped. In the second case, +the type count will be erroneously reduced on the failure path as +well. (In the first case the failure path logic has the same hole +as the reference grabbing logic.) + +Additionally, the return value of get_page() is not checked; but this +may fail even if the first get_page() succeeded due to a reference +counting overflow. + +First of all, simplify the restoration logic by explicitly counting +the reference and type references acquired. + +Consider each mapping type separately, explicitly marking the +'incoming' reference as used so we know when we need to grab a second +one. + +Finally, always check the return value of get_page[_type]() and go to +the failure path if appropriate. + +This is part of XSA-224. + +Reported-by: Jan Beulich <jbeulich@suse.com> +Signed-off-by: George Dunlap <george.dunlap@citrix.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 58 +++++++++++++++++++++++++++--------------------- + 1 file changed, 33 insertions(+), 25 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index 452538e..5e92e2c 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -758,12 +758,12 @@ __gnttab_map_grant_ref( + struct grant_table *lgt, *rgt; + struct vcpu *led; + int handle; +- unsigned long frame = 0, nr_gets = 0; ++ unsigned long frame = 0; + struct page_info *pg = NULL; + int rc = GNTST_okay; + u32 old_pin; + u32 act_pin; +- unsigned int cache_flags; ++ unsigned int cache_flags, refcnt = 0, typecnt = 0; + struct active_grant_entry *act = NULL; + struct grant_mapping *mt; + grant_entry_header_t *shah; +@@ -889,11 +889,17 @@ __gnttab_map_grant_ref( + else + owner = page_get_owner(pg); + ++ if ( owner ) ++ refcnt++; ++ + if ( !pg || (owner == dom_io) ) + { + /* Only needed the reference to confirm dom_io ownership. */ + if ( pg ) ++ { + put_page(pg); ++ refcnt--; ++ } + + if ( paging_mode_external(ld) ) + { +@@ -921,27 +927,38 @@ __gnttab_map_grant_ref( + } + else if ( owner == rd || owner == dom_cow ) + { +- if ( gnttab_host_mapping_get_page_type(op, ld, rd) ) ++ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) ) + { + if ( (owner == dom_cow) || + !get_page_type(pg, PGT_writable_page) ) + goto could_not_pin; ++ typecnt++; + } + +- nr_gets++; + if ( op->flags & GNTMAP_host_map ) + { +- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0); +- if ( rc != GNTST_okay ) +- goto undo_out; +- ++ /* ++ * Only need to grab another reference if device_map claimed ++ * the other one. ++ */ + if ( op->flags & GNTMAP_device_map ) + { +- nr_gets++; +- (void)get_page(pg, rd); +- if ( !(op->flags & GNTMAP_readonly) ) +- get_page_type(pg, PGT_writable_page); ++ if ( !get_page(pg, rd) ) ++ goto could_not_pin; ++ refcnt++; ++ } ++ ++ if ( gnttab_host_mapping_get_page_type(op, ld, rd) ) ++ { ++ if ( (owner == dom_cow) || ++ !get_page_type(pg, PGT_writable_page) ) ++ goto could_not_pin; ++ typecnt++; + } ++ ++ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0); ++ if ( rc != GNTST_okay ) ++ goto undo_out; + } + } + else +@@ -950,8 +967,6 @@ __gnttab_map_grant_ref( + if ( !rd->is_dying ) + gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n", + frame); +- if ( owner != NULL ) +- put_page(pg); + rc = GNTST_general_error; + goto undo_out; + } +@@ -1014,18 +1029,11 @@ __gnttab_map_grant_ref( + return; + + undo_out: +- if ( nr_gets > 1 ) +- { +- if ( !(op->flags & GNTMAP_readonly) ) +- put_page_type(pg); +- put_page(pg); +- } +- if ( nr_gets > 0 ) +- { +- if ( gnttab_host_mapping_get_page_type(op, ld, rd) ) +- put_page_type(pg); ++ while ( typecnt-- ) ++ put_page_type(pg); ++ ++ while ( refcnt-- ) + put_page(pg); +- } + + grant_read_lock(rgt); + +-- +2.1.4 + Added: head/emulators/xen-kernel/files/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/emulators/xen-kernel/files/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch Tue Jun 20 13:04:23 2017 (r443949) @@ -0,0 +1,319 @@ +From 3ad26b95cd9bacedad5ba503515cf6e618162be1 Mon Sep 17 00:00:00 2001 +From: Jan Beulich <jbeulich@suse.com> +Date: Thu, 15 Jun 2017 16:25:27 +0100 +Subject: [PATCH 4/4] gnttab: __gnttab_unmap_common_complete() is + all-or-nothing + +All failures have to be detected in __gnttab_unmap_common(), the +completion function must not skip part of its processing. In particular +the GNTMAP_device_map related putting of page references and adjustment +of pin count must not occur if __gnttab_unmap_common() signaled an +error. Furthermore the function must not make adjustments to global +state (here: clearing GNTTAB_device_map) before all possibly failing +operations have been performed. + +There's one exception for IOMMU related failures: As IOMMU manipulation +occurs after GNTMAP_*_map have been cleared already, the related page +reference and pin count adjustments need to be done nevertheless. A +fundamental requirement for the correctness of this is that +iommu_{,un}map_page() crash any affected DomU in case of failure. + +The version check appears to be pointless (or could perhaps be a +BUG_ON() or ASSERT()), but for the moment also move it. + +This is part of XSA-224. + +Reported-by: Jan Beulich <jbeulich@suse.com> +Signed-off-by: Jan Beulich <jbeulich@suse.com> +--- + xen/common/grant_table.c | 108 ++++++++++++++++++-------------------- + xen/include/asm-arm/grant_table.h | 2 +- + xen/include/asm-x86/grant_table.h | 5 +- + 3 files changed, 55 insertions(+), 60 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index 5e92e2c..025aad0 100644 +--- a/xen/common/grant_table.c ++++ b/xen/common/grant_table.c +@@ -96,7 +96,7 @@ struct gnttab_unmap_common { + int16_t status; + + /* Shared state beteen *_unmap and *_unmap_complete */ +- u16 flags; ++ u16 done; + unsigned long frame; + struct domain *rd; + grant_ref_t ref; +@@ -948,7 +948,8 @@ __gnttab_map_grant_ref( + refcnt++; + } + +- if ( gnttab_host_mapping_get_page_type(op, ld, rd) ) ++ if ( gnttab_host_mapping_get_page_type(op->flags & GNTMAP_readonly, ++ ld, rd) ) + { + if ( (owner == dom_cow) || + !get_page_type(pg, PGT_writable_page) ) +@@ -1095,6 +1096,7 @@ __gnttab_unmap_common( + struct active_grant_entry *act; + s16 rc = 0; + struct grant_mapping *map; ++ unsigned int flags; + bool_t put_handle = 0; + + ld = current->domain; +@@ -1145,6 +1147,20 @@ __gnttab_unmap_common( + + grant_read_lock(rgt); + ++ if ( rgt->gt_version == 0 ) ++ { ++ /* ++ * This ought to be impossible, as such a mapping should not have ++ * been established (see the nr_grant_entries(rgt) bounds check in ++ * __gnttab_map_grant_ref()). Doing this check only in ++ * __gnttab_unmap_common_complete() - as it used to be done - would, ++ * however, be too late. ++ */ ++ rc = GNTST_bad_gntref; ++ flags = 0; ++ goto unlock_out; ++ } ++ + op->rd = rd; + op->ref = map->ref; + +@@ -1160,6 +1176,7 @@ __gnttab_unmap_common( + { + gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); + rc = GNTST_bad_handle; ++ flags = 0; + goto unlock_out; + } + +@@ -1173,9 +1190,9 @@ __gnttab_unmap_common( + * hold anyway; see docs/misc/grant-tables.txt's "Locking" section. + */ + +- op->flags = read_atomic(&map->flags); ++ flags = read_atomic(&map->flags); + smp_rmb(); +- if ( unlikely(!op->flags) || unlikely(map->domid != dom) || ++ if ( unlikely(!flags) || unlikely(map->domid != dom) || + unlikely(map->ref != op->ref) ) + { + gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); +@@ -1185,24 +1202,27 @@ __gnttab_unmap_common( + + op->frame = act->frame; + +- if ( op->dev_bus_addr ) +- { +- if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) ) +- PIN_FAIL(act_release_out, GNTST_general_error, +- "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n", +- op->dev_bus_addr, pfn_to_paddr(act->frame)); +- +- map->flags &= ~GNTMAP_device_map; +- } ++ if ( op->dev_bus_addr && ++ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) ) ++ PIN_FAIL(act_release_out, GNTST_general_error, ++ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n", ++ op->dev_bus_addr, pfn_to_paddr(act->frame)); + +- if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) ++ if ( op->host_addr && (flags & GNTMAP_host_map) ) + { + if ( (rc = replace_grant_host_mapping(op->host_addr, + op->frame, op->new_addr, +- op->flags)) < 0 ) ++ flags)) < 0 ) + goto act_release_out; + + map->flags &= ~GNTMAP_host_map; ++ op->done |= GNTMAP_host_map | (flags & GNTMAP_readonly); ++ } ++ ++ if ( op->dev_bus_addr && (flags & GNTMAP_device_map) ) ++ { ++ map->flags &= ~GNTMAP_device_map; ++ op->done |= GNTMAP_device_map | (flags & GNTMAP_readonly); + } + + if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ) +@@ -1239,7 +1259,7 @@ __gnttab_unmap_common( + } + + /* If just unmapped a writable mapping, mark as dirtied */ +- if ( rc == GNTST_okay && !(op->flags & GNTMAP_readonly) ) ++ if ( rc == GNTST_okay && !(flags & GNTMAP_readonly) ) + gnttab_mark_dirty(rd, op->frame); + + op->status = rc; +@@ -1256,13 +1276,9 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + struct page_info *pg; + uint16_t *status; + +- if ( rd == NULL ) ++ if ( !op->done ) + { +- /* +- * Suggests that __gntab_unmap_common failed in +- * rcu_lock_domain_by_id() or earlier, and so we have nothing +- * to complete +- */ ++ /* __gntab_unmap_common() didn't do anything - nothing to complete. */ + return; + } + +@@ -1272,8 +1288,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + rgt = rd->grant_table; + + grant_read_lock(rgt); +- if ( rgt->gt_version == 0 ) +- goto unlock_out; + + act = active_entry_acquire(rgt, op->ref); + sha = shared_entry_header(rgt, op->ref); +@@ -1283,72 +1297,50 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) + else + status = &status_entry(rgt, op->ref); + +- if ( op->dev_bus_addr && +- unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) ) +- { +- /* +- * Suggests that __gntab_unmap_common failed early and so +- * nothing further to do +- */ +- goto act_release_out; +- } +- + pg = mfn_to_page(op->frame); + *** DIFF OUTPUT TRUNCATED AT 1000 LINES ***
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201706201304.v5KD4OsQ035765>