fix XENMEM_add_to_physmap preemption handling Just like for all other hypercalls we shouldn't be modifying the input structure - all of the fields are, even if not explicitly documented, just inputs. Signed-off-by: Jan Beulich Reviewed-by: Tim Deegan Acked-by: Keir Fraser --- v2: drop bogus ASSERT() --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -190,6 +190,9 @@ int compat_memory_op(unsigned int cmd, X break; case XENMEM_add_to_physmap: + BUILD_BUG_ON((typeof(cmp.atp.size))-1 > + (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + if ( copy_from_guest(&cmp.atp, compat, 1) ) return -EFAULT; @@ -322,17 +325,8 @@ int compat_memory_op(unsigned int cmd, X case XENMEM_current_reservation: case XENMEM_maximum_reservation: case XENMEM_maximum_gpfn: - case XENMEM_remove_from_physmap: - break; - case XENMEM_add_to_physmap: - if ( !rc || cmp.atp.space != XENMAPSPACE_gmfn_range ) - break; - - XLAT_add_to_physmap(&cmp.atp, nat.atp); - if ( __copy_to_guest(compat, &cmp.atp, 1) ) - rc = -EFAULT; - + case XENMEM_remove_from_physmap: break; default: --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -543,22 +543,29 @@ static long memory_exchange(XEN_GUEST_HA } static int xenmem_add_to_physmap(struct domain *d, - struct xen_add_to_physmap *xatp) + struct xen_add_to_physmap *xatp, + unsigned int start) { - struct xen_add_to_physmap start_xatp; - int rc = 0; + unsigned int done = 0; + long rc = 0; if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); + if ( xatp->size < start ) + return -EILSEQ; + + xatp->idx += start; + xatp->gpfn += start; + xatp->size -= start; + #ifdef HAS_PASSTHROUGH if ( need_iommu(d) ) this_cpu(iommu_dont_flush_iotlb) = 1; #endif - start_xatp = *xatp; - while ( xatp->size > 0 ) + while ( xatp->size > done ) { rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -567,12 +574,11 @@ static int xenmem_add_to_physmap(struct xatp->idx++; xatp->gpfn++; - xatp->size--; /* Check for continuation if it's not the last iteration. */ - if ( xatp->size > 0 && hypercall_preempt_check() ) + if ( xatp->size > ++done && hypercall_preempt_check() ) { - rc = -EAGAIN; + rc = start + done; break; } } @@ -581,8 +587,8 @@ static int xenmem_add_to_physmap(struct if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size); - iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size); + iommu_iotlb_flush(d, xatp->idx - done, done); + iommu_iotlb_flush(d, xatp->gpfn - done, done); } #endif @@ -594,10 +600,10 @@ long do_memory_op(unsigned long cmd, XEN struct domain *d; long rc; unsigned int address_bits; - unsigned long start_extent; struct xen_memory_reservation reservation; struct memop_args args; domid_t domid; + unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT; int op = cmd & MEMOP_CMD_MASK; switch ( op ) @@ -605,8 +611,6 @@ long do_memory_op(unsigned long cmd, XEN case XENMEM_increase_reservation: case XENMEM_decrease_reservation: case XENMEM_populate_physmap: - start_extent = cmd >> MEMOP_EXTENT_SHIFT; - if ( copy_from_guest(&reservation, arg, 1) ) return start_extent; @@ -724,6 +728,12 @@ long do_memory_op(unsigned long cmd, XEN { struct xen_add_to_physmap xatp; + BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> MEMOP_EXTENT_SHIFT)); + + /* Check for malicious or buggy input. */ + if ( start_extent != (typeof(xatp.size))start_extent ) + return -EDOM; + if ( copy_from_guest(&xatp, arg, 1) ) return -EFAULT; @@ -742,18 +752,14 @@ long do_memory_op(unsigned long cmd, XEN return rc; } - rc = xenmem_add_to_physmap(d, &xatp); + rc = xenmem_add_to_physmap(d, &xatp, start_extent); rcu_unlock_domain(d); - if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN ) - { - if ( !__copy_to_guest(arg, &xatp, 1) ) - rc = hypercall_create_continuation( - __HYPERVISOR_memory_op, "ih", op, arg); - else - rc = -EFAULT; - } + if ( xatp.space == XENMAPSPACE_gmfn_range && rc > 0 ) + rc = hypercall_create_continuation( + __HYPERVISOR_memory_op, "lh", + op | (rc << MEMOP_EXTENT_SHIFT), arg); return rc; }