[PATCH v2 2/2] KVM: Fix writeback on page boundary that propagate changes in spite of #PF

[PATCH v2 2/2] KVM: Fix writeback on page boundary that propagate changes in spite of #PF

[Nadav Amit]

Consider the case in which an instruction emulation writeback is performed on a page boundary.
In such case, if a #PF occurs on the second page, the write to the first page already occurred and cannot be retracted.
Therefore, validation of the second page access must be performed prior to writeback.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
arch/x86/kvm/x86.c |   60 +++++++++++++++++++++++++++++----------------------
1 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 05fd3d7..0e86f3a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3689,33 +3689,27 @@ static struct read_write_emulator_ops write_emultor = {
	.write = true,
};

-static int emulator_read_write_onepage(unsigned long addr, void *val,
+static int emulator_read_write_onepage(gpa_t gpa, void *val,
				       unsigned int bytes,
-				       struct x86_exception *exception,
				       struct kvm_vcpu *vcpu,
-				       struct read_write_emulator_ops *ops)
+				       struct read_write_emulator_ops *ops,
+				       bool mmio)
{
-	gpa_t gpa;
-	int handled, ret;
+	int handled;
	bool write = ops->write;

	if (ops->read_write_prepare &&
		  ops->read_write_prepare(vcpu, val, bytes))
		return X86EMUL_CONTINUE;

-	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
-
-	if (ret < 0)
-		return X86EMUL_PROPAGATE_FAULT;
-
	/* For APIC access vmexit */
-	if (ret)
-		goto mmio;
+	if (mmio)
+		goto do_mmio;

	if (ops->read_write_emulate(vcpu, gpa, val, bytes))
		return X86EMUL_CONTINUE;

-mmio:
+do_mmio:
	/*
	 * Is this MMIO handled locally?
	 */
@@ -3744,24 +3738,38 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
			struct read_write_emulator_ops *ops)
{
	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	int i, rc;
+	bool write = ops->write;
+	gpa_t gpa[2];
+	int npages = (((addr + bytes - 1) ^ addr) & PAGE_MASK) ? 2 : 1;
+	unsigned int offset[2] = {0};
+	unsigned int p_bytes[2] = {bytes, 0};
+	bool mmio[2];
+
+	if (npages == 2) {
+		p_bytes[0] = offset[1] = -addr & ~PAGE_MASK;
+		p_bytes[1] = bytes - p_bytes[0];
+	}
+
+	/* First check there is no page-fault on the next page */
+	for (i = 0; i < npages; i++) {
+		rc = vcpu_mmio_gva_to_gpa(vcpu, addr + offset[i], &gpa[i],
+			exception, write);
+		if (rc < 0)
+			return X86EMUL_PROPAGATE_FAULT;
+		mmio[i] = (rc > 0);
+	}

-	/* Crossing a page boundary? */
-	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
-		int rc, now;
-
-		now = -addr & ~PAGE_MASK;
-		rc = emulator_read_write_onepage(addr, val, now, exception,
-						 vcpu, ops);
-
+	/* Actual read/write */
+	for (i = 0; i < npages; i++) {
+		rc = emulator_read_write_onepage(gpa[i], val + offset[i],
+						 p_bytes[i], vcpu, ops,
+						 mmio[i]);
		if (rc != X86EMUL_CONTINUE)
			return rc;
-		addr += now;
-		val += now;
-		bytes -= now;
	}

-	return emulator_read_write_onepage(addr, val, bytes, exception,
-					   vcpu, ops);
+	return X86EMUL_CONTINUE;
}

static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
-- 
1.7.4.1

Re: [PATCH v2 2/2] KVM: Fix writeback on page boundary that propagate changes in spite of #PF

[Gleb Natapov]

On Sun, Jan 22, 2012 at 01:01:18PM +0200, Nadav Amit wrote:
> Consider the case in which an instruction emulation writeback is performed on a page boundary.
> In such case, if a #PF occurs on the second page, the write to the first page already occurred and cannot be retracted.
> Therefore, validation of the second page access must be performed prior to writeback.
> 
Patch is corrupted.

> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
> arch/x86/kvm/x86.c |   60 +++++++++++++++++++++++++++++----------------------
> 1 files changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 05fd3d7..0e86f3a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3689,33 +3689,27 @@ static struct read_write_emulator_ops write_emultor = {
> 	.write = true,
> };
> 
> -static int emulator_read_write_onepage(unsigned long addr, void *val,
> +static int emulator_read_write_onepage(gpa_t gpa, void *val,
> 				       unsigned int bytes,
> -				       struct x86_exception *exception,
> 				       struct kvm_vcpu *vcpu,
> -				       struct read_write_emulator_ops *ops)
> +				       struct read_write_emulator_ops *ops,
> +				       bool mmio)
> {
> -	gpa_t gpa;
> -	int handled, ret;
> +	int handled;
> 	bool write = ops->write;
> 
> 	if (ops->read_write_prepare &&
> 		  ops->read_write_prepare(vcpu, val, bytes))
> 		return X86EMUL_CONTINUE;
> 
> -	ret = vcpu_mmio_gva_to_gpa(vcpu, addr, &gpa, exception, write);
> -
> -	if (ret < 0)
> -		return X86EMUL_PROPAGATE_FAULT;
> -
> 	/* For APIC access vmexit */
> -	if (ret)
> -		goto mmio;
> +	if (mmio)
> +		goto do_mmio;
> 
> 	if (ops->read_write_emulate(vcpu, gpa, val, bytes))
> 		return X86EMUL_CONTINUE;
> 
> -mmio:
> +do_mmio:
> 	/*
> 	 * Is this MMIO handled locally?
> 	 */
> @@ -3744,24 +3738,38 @@ int emulator_read_write(struct x86_emulate_ctxt *ctxt, unsigned long addr,
> 			struct read_write_emulator_ops *ops)
> {
> 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> +	int i, rc;
> +	bool write = ops->write;
> +	gpa_t gpa[2];
> +	int npages = (((addr + bytes - 1) ^ addr) & PAGE_MASK) ? 2 : 1;
> +	unsigned int offset[2] = {0};
> +	unsigned int p_bytes[2] = {bytes, 0};
> +	bool mmio[2];
> +
> +	if (npages == 2) {
> +		p_bytes[0] = offset[1] = -addr & ~PAGE_MASK;
> +		p_bytes[1] = bytes - p_bytes[0];
> +	}
> +
> +	/* First check there is no page-fault on the next page */
> +	for (i = 0; i < npages; i++) {
> +		rc = vcpu_mmio_gva_to_gpa(vcpu, addr + offset[i], &gpa[i],
> +			exception, write);
> +		if (rc < 0)
> +			return X86EMUL_PROPAGATE_FAULT;
> +		mmio[i] = (rc > 0);
> +	}
> 
> -	/* Crossing a page boundary? */
> -	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
> -		int rc, now;
> -
> -		now = -addr & ~PAGE_MASK;
> -		rc = emulator_read_write_onepage(addr, val, now, exception,
> -						 vcpu, ops);
> -
> +	/* Actual read/write */
> +	for (i = 0; i < npages; i++) {
> +		rc = emulator_read_write_onepage(gpa[i], val + offset[i],
> +						 p_bytes[i], vcpu, ops,
> +						 mmio[i]);
> 		if (rc != X86EMUL_CONTINUE)
> 			return rc;
> -		addr += now;
> -		val += now;
> -		bytes -= now;
> 	}
> 
> -	return emulator_read_write_onepage(addr, val, bytes, exception,
> -					   vcpu, ops);
> +	return X86EMUL_CONTINUE;
> }
> 
> static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
> -- 
> 1.7.4.1
> 

--
			Gleb.