linux-kernel - Re: [Xen-devel] [V1 PATCH] dom0 pvh: map foreign pfns in our p2m for toolstack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 May 2014 11:59:26 +0100
From:	David Vrabel <david.vrabel@...rix.com>
To:	Roger Pau Monné <roger.pau@...rix.com>,
	Mukesh Rathor <mukesh.rathor@...cle.com>,
	<boris.ostrovsky@...cle.com>
CC:	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [Xen-devel] [V1 PATCH] dom0 pvh: map foreign pfns in our p2m
 for toolstack

On 27/05/14 11:43, Roger Pau Monné wrote:
> On 24/05/14 03:33, Mukesh Rathor wrote:
>> When running as dom0 in pvh mode, foreign pfns that are accessed must be
>> added to our p2m which is managed by xen. This is done via
>> XENMEM_add_to_physmap_range hypercall. This is needed for toolstack
>> building guests and mapping guest memory, xentrace mapping xen pages,
>> etc..

Thanks.

Applied to devel/for-linus-3.16, but see comments below.

>> +static int xlate_add_to_p2m(unsigned long lpfn, unsigned long fgmfn,
>> +			    unsigned int domid)

The preferred abbreviation is GFN not GMFN.  I fixed this up.

>> +{
>> +	int rc, err = 0;
>> +	xen_pfn_t gpfn = lpfn;
>> +	xen_ulong_t idx = fgmfn;
>> +
>> +	struct xen_add_to_physmap_range xatp = {
>> +		.domid = DOMID_SELF,
>> +		.foreign_domid = domid,
>> +		.size = 1,
>> +		.space = XENMAPSPACE_gmfn_foreign,
>> +	};
>> +	set_xen_guest_handle(xatp.idxs, &idx);
>> +	set_xen_guest_handle(xatp.gpfns, &gpfn);
>> +	set_xen_guest_handle(xatp.errs, &err);
>> +
>> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
>> +	return rc;
> 
> Thanks for the patches, I see two problems with this approach, the first
> one is that you are completely ignoring the error in the variable "err",
> which means that you can end up with a pfn that Linux thinks it's valid,
> but it's not mapped to any mfn, so when you try to access it you will
> trigger the vioapic crash.

I spotted this and fixed this up by adding:

+       if (rc < 0)
+               return rc;
+       return err;

> The second one is that this seems extremely inefficient, you are issuing
> one hypercall for each memory page, when you could instead batch all the
> pages into a single hypercall and map them in one shot.

I agree, but the 3.16 merge window is nearly here so I've applied it
as-is.  Note that the privcmd driver calls this function once per page,
so the lack of batching doesn't really hurt here.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/