[PATCH v2 0/2] Fix KVM_GET_REG_LIST invalid register ID regression

[PATCH v2 0/2] Fix KVM_GET_REG_LIST invalid register ID regression

[Dave Martin]

Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
that do not correspond to a single underlying architectural register.

This series proposed a fix for this regression along with some related
refactoring.

Comments welcome.

This series now looks straightforward, but has only been build-tested
so far.


Dave Martin (2):
  KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST
  KVM: arm64: Factor out KVM_GET_REG_LIST core register enumeration

 arch/arm64/kvm/guest.c | 74 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 12 deletions(-)

-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Dave Martin]

Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
that do not correspond to a single underlying architectural register.

KVM_GET_REG_LIST was not changed to match however: instead, it
simply yields a list of 32-bit register IDs that together cover the
whole kvm_regs struct.  This means that if userspace tries to use
the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
some of those calls will now fail.

This was not the intention.  Instead, iterating KVM_*_ONE_REG over
the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
to work.

This patch fixes the problem by splitting validate_core_reg_id()
into a backend core_reg_size_from_offset() which does all of the
work except for checking that the size field in the register ID
matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
converted to use this to enumerate the valid offsets.

kvm_arm_copy_reg_indices() now also sets the register ID size field
appropriately based on the value returned, so the register ID
supplied to userspace is fully qualified for use with the register
access ioctls.

Cc: stable@vger.kernel.org
Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from userspace")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 61 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index dd436a5..cbe423b 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -57,9 +57,8 @@ static u64 core_reg_offset_from_id(u64 id)
 	return id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_CORE);
 }
 
-static int validate_core_offset(const struct kvm_one_reg *reg)
+static int core_reg_size_from_offset(u64 off)
 {
-	u64 off = core_reg_offset_from_id(reg->id);
 	int size;
 
 	switch (off) {
@@ -89,8 +88,21 @@ static int validate_core_offset(const struct kvm_one_reg *reg)
 		return -EINVAL;
 	}
 
-	if (KVM_REG_SIZE(reg->id) == size &&
-	    IS_ALIGNED(off, size / sizeof(__u32)))
+	if (IS_ALIGNED(off, size / sizeof(__u32)))
+		return size;
+
+	return -EINVAL;
+}
+
+static int validate_core_offset(const struct kvm_one_reg *reg)
+{
+	u64 off = core_reg_offset_from_id(reg->id);
+	int size = core_reg_size_from_offset(off);
+
+	if (size < 0)
+		return -EINVAL;
+
+	if (KVM_REG_SIZE(reg->id) == size)
 		return 0;
 
 	return -EINVAL;
@@ -195,7 +207,19 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 
 static unsigned long num_core_regs(void)
 {
-	return sizeof(struct kvm_regs) / sizeof(__u32);
+	unsigned int i;
+	int n = 0;
+
+	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
+		int size = core_reg_size_from_offset(i);
+
+		if (size < 0)
+			continue;
+
+		n++;
+	}
+
+	return n;
 }
 
 /**
@@ -270,11 +294,34 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
 	unsigned int i;
-	const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE;
 	int ret;
 
 	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
-		if (put_user(core_reg | i, uindices))
+		u64 reg = KVM_REG_ARM64 | KVM_REG_ARM_CORE | i;
+		int size = core_reg_size_from_offset(i);
+
+		if (size < 0)
+			continue;
+
+		switch (size) {
+		case sizeof(__u32):
+			reg |= KVM_REG_SIZE_U32;
+			break;
+
+		case sizeof(__u64):
+			reg |= KVM_REG_SIZE_U64;
+			break;
+
+		case sizeof(__uint128_t):
+			reg |= KVM_REG_SIZE_U128;
+			break;
+
+		default:
+			WARN_ON(1);
+			continue;
+		}
+
+		if (put_user(reg, uindices))
 			return -EFAULT;
 		uindices++;
 	}
-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Dave Martin]

On Wed, Dec 12, 2018 at 08:17:03PM +0000, Dave Martin wrote:
> Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
> access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
> that do not correspond to a single underlying architectural register.
> 
> KVM_GET_REG_LIST was not changed to match however: instead, it
> simply yields a list of 32-bit register IDs that together cover the
> whole kvm_regs struct.  This means that if userspace tries to use
> the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
> some of those calls will now fail.
> 
> This was not the intention.  Instead, iterating KVM_*_ONE_REG over
> the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
> to work.
> 
> This patch fixes the problem by splitting validate_core_reg_id()
> into a backend core_reg_size_from_offset() which does all of the
> work except for checking that the size field in the register ID
> matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
> converted to use this to enumerate the valid offsets.
> 
> kvm_arm_copy_reg_indices() now also sets the register ID size field
> appropriately based on the value returned, so the register ID
> supplied to userspace is fully qualified for use with the register
> access ioctls.
> 
> Cc: stable@vger.kernel.org
> Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from userspace")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Tested now with [1], which obtains the reg list with KVM_GET_REG_LIST
and then tries to read each register listed.

(Comparing v4.19 with a patches v4.20-rc5 was a bit lazy here, but there
is no reason to suppose the results would be different.)


This confirms both the exactly expected bug behaviour and the fix.


I have not yet checked what qemu does with the KVM_GET_REG_LIST data.

Cheers
---Dave

--8<--

--- reglist-4.19.0	2018-12-13 18:14:03.970008568 +0000
+++ reglist-4.20.0-rc5-00001-gc80a4e51b4d3	2018-12-13 17:50:08.281045482 +0000
@@ -1,461 +1,321 @@
 KVM_GET_REG_LIST: Exec format error
 KVM_ARM_PREFERRED_TARGET:
 	target = 5
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	features[0] = 0x00000000
 	0x6030000000100000	OK
-	0x6030000000100001	FAIL (Invalid argument)
 	0x6030000000100002	OK
-	0x6030000000100003	FAIL (Invalid argument)
 	0x6030000000100004	OK
-	0x6030000000100005	FAIL (Invalid argument)
 	0x6030000000100006	OK
-	0x6030000000100007	FAIL (Invalid argument)
 	0x6030000000100008	OK
-	0x6030000000100009	FAIL (Invalid argument)
 	0x603000000010000a	OK
-	0x603000000010000b	FAIL (Invalid argument)
 	0x603000000010000c	OK
-	0x603000000010000d	FAIL (Invalid argument)
 	0x603000000010000e	OK
-	0x603000000010000f	FAIL (Invalid argument)
 	0x6030000000100010	OK
-	0x6030000000100011	FAIL (Invalid argument)
 	0x6030000000100012	OK
-	0x6030000000100013	FAIL (Invalid argument)
 	0x6030000000100014	OK
-	0x6030000000100015	FAIL (Invalid argument)
 	0x6030000000100016	OK
-	0x6030000000100017	FAIL (Invalid argument)
 	0x6030000000100018	OK
-	0x6030000000100019	FAIL (Invalid argument)
 	0x603000000010001a	OK
-	0x603000000010001b	FAIL (Invalid argument)
 	0x603000000010001c	OK
-	0x603000000010001d	FAIL (Invalid argument)
 	0x603000000010001e	OK
-	0x603000000010001f	FAIL (Invalid argument)
 	0x6030000000100020	OK
-	0x6030000000100021	FAIL (Invalid argument)
 	0x6030000000100022	OK
-	0x6030000000100023	FAIL (Invalid argument)
 	0x6030000000100024	OK
-	0x6030000000100025	FAIL (Invalid argument)
 	0x6030000000100026	OK
-	0x6030000000100027	FAIL (Invalid argument)
 	0x6030000000100028	OK
-	0x6030000000100029	FAIL (Invalid argument)
 	0x603000000010002a	OK
-	0x603000000010002b	FAIL (Invalid argument)
 	0x603000000010002c	OK
-	0x603000000010002d	FAIL (Invalid argument)
 	0x603000000010002e	OK
-	0x603000000010002f	FAIL (Invalid argument)
 	0x6030000000100030	OK
-	0x6030000000100031	FAIL (Invalid argument)
 	0x6030000000100032	OK
-	0x6030000000100033	FAIL (Invalid argument)
 	0x6030000000100034	OK
-	0x6030000000100035	FAIL (Invalid argument)
 	0x6030000000100036	OK
-	0x6030000000100037	FAIL (Invalid argument)
 	0x6030000000100038	OK
-	0x6030000000100039	FAIL (Invalid argument)
 	0x603000000010003a	OK
-	0x603000000010003b	FAIL (Invalid argument)
 	0x603000000010003c	OK
-	0x603000000010003d	FAIL (Invalid argument)
 	0x603000000010003e	OK
-	0x603000000010003f	FAIL (Invalid argument)
 	0x6030000000100040	OK
-	0x6030000000100041	FAIL (Invalid argument)
 	0x6030000000100042	OK
-	0x6030000000100043	FAIL (Invalid argument)
 	0x6030000000100044	OK
-	0x6030000000100045	FAIL (Invalid argument)
 	0x6030000000100046	OK
-	0x6030000000100047	FAIL (Invalid argument)
 	0x6030000000100048	OK
-	0x6030000000100049	FAIL (Invalid argument)
 	0x603000000010004a	OK
-	0x603000000010004b	FAIL (Invalid argument)
 	0x603000000010004c	OK
-	0x603000000010004d	FAIL (Invalid argument)
 	0x603000000010004e	OK
-	0x603000000010004f	FAIL (Invalid argument)
 	0x6030000000100050	OK
-	0x6030000000100051	FAIL (Invalid argument)
-	0x6030000000100052	FAIL (Invalid argument)
-	0x6030000000100053	FAIL (Invalid argument)
-	0x6030000000100054	FAIL (Invalid argument)
-	0x6030000000100055	FAIL (Invalid argument)
-	0x6030000000100056	FAIL (Invalid argument)
-	0x6030000000100057	FAIL (Invalid argument)
-	0x6030000000100058	FAIL (Invalid argument)
-	0x6030000000100059	FAIL (Invalid argument)
-	0x603000000010005a	FAIL (Invalid argument)
-	0x603000000010005b	FAIL (Invalid argument)
-	0x603000000010005c	FAIL (Invalid argument)
-	0x603000000010005d	FAIL (Invalid argument)
-	0x603000000010005e	FAIL (Invalid argument)
-	0x603000000010005f	FAIL (Invalid argument)
-	0x6030000000100060	FAIL (Invalid argument)
-	0x6030000000100061	FAIL (Invalid argument)
-	0x6030000000100062	FAIL (Invalid argument)
-	0x6030000000100063	FAIL (Invalid argument)
-	0x6030000000100064	FAIL (Invalid argument)
-	0x6030000000100065	FAIL (Invalid argument)
-	0x6030000000100066	FAIL (Invalid argument)
-	0x6030000000100067	FAIL (Invalid argument)
-	0x6030000000100068	FAIL (Invalid argument)
-	0x6030000000100069	FAIL (Invalid argument)
-	0x603000000010006a	FAIL (Invalid argument)
-	0x603000000010006b	FAIL (Invalid argument)
-	0x603000000010006c	FAIL (Invalid argument)
-	0x603000000010006d	FAIL (Invalid argument)
-	0x603000000010006e	FAIL (Invalid argument)
-	0x603000000010006f	FAIL (Invalid argument)
-	0x6030000000100070	FAIL (Invalid argument)
-	0x6030000000100071	FAIL (Invalid argument)
-	0x6030000000100072	FAIL (Invalid argument)
-	0x6030000000100073	FAIL (Invalid argument)
-	0x6030000000100074	FAIL (Invalid argument)
-	0x6030000000100075	FAIL (Invalid argument)
-	0x6030000000100076	FAIL (Invalid argument)
-	0x6030000000100077	FAIL (Invalid argument)
-	0x6030000000100078	FAIL (Invalid argument)
-	0x6030000000100079	FAIL (Invalid argument)
-	0x603000000010007a	FAIL (Invalid argument)
-	0x603000000010007b	FAIL (Invalid argument)
-	0x603000000010007c	FAIL (Invalid argument)
-	0x603000000010007d	FAIL (Invalid argument)
-	0x603000000010007e	FAIL (Invalid argument)
-	0x603000000010007f	FAIL (Invalid argument)
-	0x6030000000100080	FAIL (Invalid argument)
-	0x6030000000100081	FAIL (Invalid argument)
-	0x6030000000100082	FAIL (Invalid argument)
-	0x6030000000100083	FAIL (Invalid argument)
-	0x6030000000100084	FAIL (Invalid argument)
-	0x6030000000100085	FAIL (Invalid argument)
-	0x6030000000100086	FAIL (Invalid argument)
-	0x6030000000100087	FAIL (Invalid argument)
-	0x6030000000100088	FAIL (Invalid argument)
-	0x6030000000100089	FAIL (Invalid argument)
-	0x603000000010008a	FAIL (Invalid argument)
-	0x603000000010008b	FAIL (Invalid argument)
-	0x603000000010008c	FAIL (Invalid argument)
-	0x603000000010008d	FAIL (Invalid argument)
-	0x603000000010008e	FAIL (Invalid argument)
-	0x603000000010008f	FAIL (Invalid argument)
-	0x6030000000100090	FAIL (Invalid argument)
-	0x6030000000100091	FAIL (Invalid argument)
-	0x6030000000100092	FAIL (Invalid argument)
-	0x6030000000100093	FAIL (Invalid argument)
-	0x6030000000100094	FAIL (Invalid argument)
-	0x6030000000100095	FAIL (Invalid argument)
-	0x6030000000100096	FAIL (Invalid argument)
-	0x6030000000100097	FAIL (Invalid argument)
-	0x6030000000100098	FAIL (Invalid argument)
-	0x6030000000100099	FAIL (Invalid argument)
-	0x603000000010009a	FAIL (Invalid argument)
-	0x603000000010009b	FAIL (Invalid argument)
-	0x603000000010009c	FAIL (Invalid argument)
-	0x603000000010009d	FAIL (Invalid argument)
-	0x603000000010009e	FAIL (Invalid argument)
-	0x603000000010009f	FAIL (Invalid argument)
-	0x60300000001000a0	FAIL (Invalid argument)
-	0x60300000001000a1	FAIL (Invalid argument)
-	0x60300000001000a2	FAIL (Invalid argument)
-	0x60300000001000a3	FAIL (Invalid argument)
-	0x60300000001000a4	FAIL (Invalid argument)
-	0x60300000001000a5	FAIL (Invalid argument)
-	0x60300000001000a6	FAIL (Invalid argument)
-	0x60300000001000a7	FAIL (Invalid argument)
-	0x60300000001000a8	FAIL (Invalid argument)
-	0x60300000001000a9	FAIL (Invalid argument)
-	0x60300000001000aa	FAIL (Invalid argument)
-	0x60300000001000ab	FAIL (Invalid argument)
-	0x60300000001000ac	FAIL (Invalid argument)
-	0x60300000001000ad	FAIL (Invalid argument)
-	0x60300000001000ae	FAIL (Invalid argument)
-	0x60300000001000af	FAIL (Invalid argument)
-	0x60300000001000b0	FAIL (Invalid argument)
-	0x60300000001000b1	FAIL (Invalid argument)
-	0x60300000001000b2	FAIL (Invalid argument)
-	0x60300000001000b3	FAIL (Invalid argument)
-	0x60300000001000b4	FAIL (Invalid argument)
-	0x60300000001000b5	FAIL (Invalid argument)
-	0x60300000001000b6	FAIL (Invalid argument)
-	0x60300000001000b7	FAIL (Invalid argument)
-	0x60300000001000b8	FAIL (Invalid argument)
-	0x60300000001000b9	FAIL (Invalid argument)
-	0x60300000001000ba	FAIL (Invalid argument)
-	0x60300000001000bb	FAIL (Invalid argument)
-	0x60300000001000bc	FAIL (Invalid argument)
-	0x60300000001000bd	FAIL (Invalid argument)
-	0x60300000001000be	FAIL (Invalid argument)
-	0x60300000001000bf	FAIL (Invalid argument)
-	0x60300000001000c0	FAIL (Invalid argument)
-	0x60300000001000c1	FAIL (Invalid argument)
-	0x60300000001000c2	FAIL (Invalid argument)
-	0x60300000001000c3	FAIL (Invalid argument)
-	0x60300000001000c4	FAIL (Invalid argument)
-	0x60300000001000c5	FAIL (Invalid argument)
-	0x60300000001000c6	FAIL (Invalid argument)
-	0x60300000001000c7	FAIL (Invalid argument)
-	0x60300000001000c8	FAIL (Invalid argument)
-	0x60300000001000c9	FAIL (Invalid argument)
-	0x60300000001000ca	FAIL (Invalid argument)
-	0x60300000001000cb	FAIL (Invalid argument)
-	0x60300000001000cc	FAIL (Invalid argument)
-	0x60300000001000cd	FAIL (Invalid argument)
-	0x60300000001000ce	FAIL (Invalid argument)
-	0x60300000001000cf	FAIL (Invalid argument)
-	0x60300000001000d0	FAIL (Invalid argument)
-	0x60300000001000d1	FAIL (Invalid argument)
-	0x60300000001000d2	FAIL (Invalid argument)
-	0x60300000001000d3	FAIL (Invalid argument)
-	0x60300000001000d4	FAIL (Invalid argument)
-	0x60300000001000d5	FAIL (Invalid argument)
-	0x60300000001000d6	FAIL (No such file or directory)
-	0x60300000001000d7	FAIL (No such file or directory)
+	0x6040000000100054	OK
+	0x6040000000100058	OK
+	0x604000000010005c	OK
+	0x6040000000100060	OK
+	0x6040000000100064	OK
+	0x6040000000100068	OK
+	0x604000000010006c	OK
+	0x6040000000100070	OK
+	0x6040000000100074	OK
+	0x6040000000100078	OK
+	0x604000000010007c	OK
+	0x6040000000100080	OK
+	0x6040000000100084	OK
+	0x6040000000100088	OK
+	0x604000000010008c	OK
+	0x6040000000100090	OK
+	0x6040000000100094	OK
+	0x6040000000100098	OK
+	0x604000000010009c	OK
+	0x60400000001000a0	OK
+	0x60400000001000a4	OK
+	0x60400000001000a8	OK
+	0x60400000001000ac	OK
+	0x60400000001000b0	OK
+	0x60400000001000b4	OK
+	0x60400000001000b8	OK
+	0x60400000001000bc	OK
+	0x60400000001000c0	OK
+	0x60400000001000c4	OK
+	0x60400000001000c8	OK
+	0x60400000001000cc	OK
+	0x60400000001000d0	OK
+	0x60200000001000d4	OK
+	0x60200000001000d5	OK
 	0x6030000000140000	OK
 	0x603000000013df19	OK
 	0x603000000013df1a	OK
 	0x603000000013df02	OK
 	0x603000000013c000	OK
 	0x603000000013c006	OK
 	0x603000000013c801	OK
 	0x603000000013c807	OK
 	0x603000000013d801	OK
 	0x6030000000138004	OK
 	0x6030000000138005	OK
 	0x6030000000138006	OK
 	0x6030000000138007	OK
 	0x603000000013800c	OK
 	0x603000000013800d	OK
 	0x603000000013800e	OK
 	0x603000000013800f	OK
 	0x6030000000138010	OK
 	0x6030000000138012	OK
 	0x6030000000138014	OK
 	0x6030000000138015	OK
 	0x6030000000138016	OK
 	0x6030000000138017	OK
 	0x603000000013801c	OK
 	0x603000000013801d	OK
 	0x603000000013801e	OK
 	0x603000000013801f	OK
 	0x6030000000138024	OK
 	0x6030000000138025	OK
 	0x6030000000138026	OK
 	0x6030000000138027	OK
 	0x603000000013802c	OK
 	0x603000000013802d	OK
 	0x603000000013802e	OK
 	0x603000000013802f	OK
 	0x6030000000138034	OK
 	0x6030000000138035	OK
 	0x6030000000138036	OK
 	0x6030000000138037	OK
 	0x603000000013803c	OK
 	0x603000000013803d	OK
 	0x603000000013803e	OK
 	0x603000000013803f	OK
 	0x6030000000138044	OK
 	0x6030000000138045	OK
 	0x6030000000138046	OK
 	0x6030000000138047	OK
 	0x603000000013804c	OK
 	0x603000000013804d	OK
 	0x603000000013804e	OK
 	0x603000000013804f	OK
 	0x6030000000138054	OK
 	0x6030000000138055	OK
 	0x6030000000138056	OK
 	0x6030000000138057	OK
 	0x603000000013805c	OK
 	0x603000000013805d	OK
 	0x603000000013805e	OK
 	0x603000000013805f	OK
 	0x6030000000138064	OK
 	0x6030000000138065	OK
 	0x6030000000138066	OK
 	0x6030000000138067	OK
 	0x603000000013806c	OK
 	0x603000000013806d	OK
 	0x603000000013806e	OK
 	0x603000000013806f	OK
 	0x6030000000138074	OK
 	0x6030000000138075	OK
 	0x6030000000138076	OK
 	0x6030000000138077	OK
 	0x603000000013807c	OK
 	0x603000000013807d	OK
 	0x603000000013807e	OK
 	0x603000000013807f	OK
 	0x603000000013a038	OK
 	0x603000000013c005	OK
 	0x603000000013c008	OK
 	0x603000000013c009	OK
 	0x603000000013c00a	OK
 	0x603000000013c00b	OK
 	0x603000000013c00c	OK
 	0x603000000013c00d	OK
 	0x603000000013c00e	OK
 	0x603000000013c00f	OK
 	0x603000000013c010	OK
 	0x603000000013c011	OK
 	0x603000000013c012	OK
 	0x603000000013c013	OK
 	0x603000000013c014	OK
 	0x603000000013c015	OK
 	0x603000000013c016	OK
 	0x603000000013c017	OK
 	0x603000000013c018	OK
 	0x603000000013c019	OK
 	0x603000000013c01a	OK
 	0x603000000013c01b	OK
 	0x603000000013c01c	OK
 	0x603000000013c01d	OK
 	0x603000000013c01e	OK
 	0x603000000013c01f	OK
 	0x603000000013c020	OK
 	0x603000000013c021	OK
 	0x603000000013c022	OK
 	0x603000000013c023	OK
 	0x603000000013c024	OK
 	0x603000000013c025	OK
 	0x603000000013c026	OK
 	0x603000000013c027	OK
 	0x603000000013c028	OK
 	0x603000000013c029	OK
 	0x603000000013c02a	OK
 	0x603000000013c02b	OK
 	0x603000000013c02c	OK
 	0x603000000013c02d	OK
 	0x603000000013c02e	OK
 	0x603000000013c02f	OK
 	0x603000000013c030	OK
 	0x603000000013c031	OK
 	0x603000000013c032	OK
 	0x603000000013c033	OK
 	0x603000000013c034	OK
 	0x603000000013c035	OK
 	0x603000000013c036	OK
 	0x603000000013c037	OK
 	0x603000000013c038	OK
 	0x603000000013c039	OK
 	0x603000000013c03a	OK
 	0x603000000013c03b	OK
 	0x603000000013c03c	OK
 	0x603000000013c03d	OK
 	0x603000000013c03e	OK
 	0x603000000013c03f	OK
 	0x603000000013c080	OK
 	0x603000000013c081	OK
 	0x603000000013c082	OK
 	0x603000000013c100	OK
 	0x603000000013c101	OK
 	0x603000000013c102	OK
 	0x603000000013c288	OK
 	0x603000000013c289	OK
 	0x603000000013c290	OK
 	0x603000000013c300	OK
 	0x603000000013c3a0	OK
 	0x603000000013c4f1	OK
 	0x603000000013c4f2	OK
 	0x603000000013c510	OK
 	0x603000000013c518	OK
 	0x603000000013c600	OK
 	0x603000000013c609	OK
 	0x603000000013c681	OK
 	0x603000000013c684	OK
 	0x603000000013c708	OK
 	0x603000000013d000	OK
 	0x603000000013dce1	OK
 	0x603000000013dce2	OK
 	0x603000000013dce3	OK
 	0x603000000013dce4	OK
 	0x603000000013dce5	OK
 	0x603000000013dce8	OK
 	0x603000000013dcf0	OK
 	0x603000000013dcf3	OK
 	0x603000000013de82	OK
 	0x603000000013de83	OK
 	0x603000000013df40	OK
 	0x603000000013df41	OK
 	0x603000000013df42	OK
 	0x603000000013df43	OK
 	0x603000000013df44	OK
 	0x603000000013df45	OK
 	0x603000000013df46	OK
 	0x603000000013df47	OK
 	0x603000000013df48	OK
 	0x603000000013df49	OK
 	0x603000000013df4a	OK
 	0x603000000013df4b	OK
 	0x603000000013df4c	OK
 	0x603000000013df4d	OK
 	0x603000000013df4e	OK
 	0x603000000013df4f	OK
 	0x603000000013df50	OK
 	0x603000000013df51	OK
 	0x603000000013df52	OK
 	0x603000000013df53	OK
 	0x603000000013df54	OK
 	0x603000000013df55	OK
 	0x603000000013df56	OK
 	0x603000000013df57	OK
 	0x603000000013df58	OK
 	0x603000000013df59	OK
 	0x603000000013df5a	OK
 	0x603000000013df5b	OK
 	0x603000000013df5c	OK
 	0x603000000013df5d	OK
 	0x603000000013df5e	OK
 	0x603000000013df60	OK
 	0x603000000013df61	OK
 	0x603000000013df62	OK
 	0x603000000013df63	OK
 	0x603000000013df64	OK
 	0x603000000013df65	OK
 	0x603000000013df66	OK
 	0x603000000013df67	OK
 	0x603000000013df68	OK
 	0x603000000013df69	OK
 	0x603000000013df6a	OK
 	0x603000000013df6b	OK
 	0x603000000013df6c	OK
 	0x603000000013df6d	OK
 	0x603000000013df6e	OK
 	0x603000000013df6f	OK
 	0x603000000013df70	OK
 	0x603000000013df71	OK
 	0x603000000013df72	OK
 	0x603000000013df73	OK
 	0x603000000013df74	OK
 	0x603000000013df75	OK
 	0x603000000013df76	OK
 	0x603000000013df77	OK
 	0x603000000013df78	OK
 	0x603000000013df79	OK
 	0x603000000013df7a	OK
 	0x603000000013df7b	OK
 	0x603000000013df7c	OK
 	0x603000000013df7d	OK
 	0x603000000013df7e	OK
 	0x603000000013df7f	OK
 	0x603000000013e180	OK
 	0x603000000013e281	OK
 	0x603000000013e298	OK
 	0x6020000000110000	OK
 	0x6020000000110001	OK
 	0x6020000000110002	OK
 	0x6020000000110004	OK
-FAIL
+
+PASS

[1]

#include <assert.h>
#include <errno.h>
#include <limits.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/kvm.h>

#define KVM_PATH "/dev/kvm"

#define ARRAY_SIZE(array) (sizeof (array) / sizeof *(array))

static int get_reg_list(int vcpu, struct kvm_reg_list **pregs, size_t *psize)
{
	struct kvm_reg_list *regs = *pregs;
	size_t num_needed = 0;
	const size_t reg_offset = (char *)&regs->reg - (char *)regs;

	while (1) {
		size_t size_needed = num_needed * sizeof regs->reg[0]
					+ reg_offset;
		if (size_needed > *psize) {
			regs = realloc(regs, size_needed);
			if (!regs)
				goto nomem;

			*pregs = regs;
			*psize = size_needed;
		}

		regs->n = (*psize - reg_offset) / sizeof regs->reg[0];
		if (!ioctl(vcpu, KVM_GET_REG_LIST, regs))
			break;

		if (errno != E2BIG) {
			perror("KVM_GET_REG_LIST");
			goto error;
		}

		num_needed = regs->n;
	}

	return 0;	

nomem:
	errno = ENOMEM;
	perror(NULL);
error:
	return -1;
}

int main(void)
{
	int fail = 0;
	int kvm_fd, vm, vcpu;
	struct kvm_vcpu_init init;
	size_t i;
	struct kvm_reg_list *regs = NULL;
	size_t regs_size = 0;
	uint64_t regdata[2];
	struct kvm_one_reg reg;

	kvm_fd = open(KVM_PATH, O_RDWR);
	if (kvm_fd == -1) {
		perror(KVM_PATH);
		goto error;
	}

	vm = ioctl(kvm_fd, KVM_CREATE_VM, 0);
	if (vm == -1) {
		perror("KVM_CREATE_VM");
		goto error;
	}

	if (ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init)) {
		perror("KVM_ARM_PREFERRED_TARGET");
		goto error;
	}

	printf("KVM_ARM_PREFERRED_TARGET:\n"
	       "\ttarget = %lu\n",
	       (unsigned long)init.target);
	for (i = 0; i < ARRAY_SIZE(init.features); ++i)
		printf("\tfeatures[0] = 0x%.8lx\n",
		       (unsigned long)init.features[i]);
	
	vcpu = ioctl(vm, KVM_CREATE_VCPU, 0);
	if (vcpu == -1) {
		perror("KVM_CREATE_VCPU");
		goto error;
	}

	if (!get_reg_list(vcpu, &regs, &regs_size)) {
		fputs("Strange, KVM_GET_REG_LIST succeeded before KVM_VCPU_INIT!\n",
		      stderr);
			
		goto error;
	}

	if (ioctl(vcpu, KVM_ARM_VCPU_INIT, &init)) {
		perror("KVM_ARM_VCPU_INIT");
		goto error;
	}

	if (get_reg_list(vcpu, &regs, &regs_size))
		goto error;

	assert(1 <= -1ULL >> 32);
	assert(ULONG_MAX >= (1ULL << 32) - 1);
	for (i = 0; i < regs->n; ++i) {
		size_t size;
		char const *s1 = "OK", *s2 = "", *s3 = "";;

		size = (size_t)1 << ((regs->reg[i] & KVM_REG_SIZE_MASK) >>
				     KVM_REG_SIZE_SHIFT);
		if (size < 4 || size > 16) {
			s1 = "BADSIZE";
			fail = 1;
		} else {
			memset(&reg, 0, sizeof reg);

			reg.id = regs->reg[i];
			reg.addr = (__u64)&regdata;
			if (ioctl(vcpu, KVM_GET_ONE_REG, &reg)) {
				s1 = "FAIL (";
				s2 = strerror(errno);
				s3 = ")";
				fail = 1;
			}
		}

		printf("\t0x%.16lx\t%s%s%s\n",
		       (unsigned long)reg.id, s1, s2, s3);
	
	}

	if (ferror(stdout) || fflush(stdout))
		goto stdout_error;
	
	if (fail) {
		puts("FAIL");
		goto error;
	}

	if (puts("\nPASS") == EOF || fclose(stdout))
		goto stdout_error;

	return EXIT_SUCCESS;

stdout_error:
	fputs("I/O error", stderr);
error:
	return EXIT_FAILURE;
}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Dave Martin]

On Thu, Dec 13, 2018 at 06:30:39PM +0000, Dave Martin wrote:
> On Wed, Dec 12, 2018 at 08:17:03PM +0000, Dave Martin wrote:
> > Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
> > access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
> > that do not correspond to a single underlying architectural register.
> > 
> > KVM_GET_REG_LIST was not changed to match however: instead, it
> > simply yields a list of 32-bit register IDs that together cover the
> > whole kvm_regs struct.  This means that if userspace tries to use
> > the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
> > some of those calls will now fail.
> > 
> > This was not the intention.  Instead, iterating KVM_*_ONE_REG over
> > the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
> > to work.
> > 
> > This patch fixes the problem by splitting validate_core_reg_id()
> > into a backend core_reg_size_from_offset() which does all of the
> > work except for checking that the size field in the register ID
> > matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
> > converted to use this to enumerate the valid offsets.
> > 
> > kvm_arm_copy_reg_indices() now also sets the register ID size field
> > appropriately based on the value returned, so the register ID
> > supplied to userspace is fully qualified for use with the register
> > access ioctls.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from userspace")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> Tested now with [1], which obtains the reg list with KVM_GET_REG_LIST
> and then tries to read each register listed.
> 
> (Comparing v4.19 with a patches v4.20-rc5 was a bit lazy here, but there
> is no reason to suppose the results would be different.)
> 
> 
> This confirms both the exactly expected bug behaviour and the fix.
> 
> 
> I have not yet checked what qemu does with the KVM_GET_REG_LIST data.

Further to this, qemu seems only to use the non-KVM_REG_ARM_CORE
registers from the KVM_GET_REG_LIST output, and uses its own built-in
knowledge to enumerate the core regs (since that is a fixed set anyway).

qemu already explicitly marks core regs with the correct size (32-/64-
or 128-bit) when doing KVM_GET_ONE_REG/KVM_SET_ONE_REG.  So it shouldn't
be affected by this patch.

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Andrew Jones]

Hi Dave,

The below looks like a nice kvm selftest. Care to fit it into that
framework (tools/testing/selftests/kvm/) and post it?

Thanks,
drew

> [1]
> 
> #include <assert.h>
> #include <errno.h>
> #include <limits.h>
> #include <stddef.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdint.h>
> #include <string.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
> 
> #define KVM_PATH "/dev/kvm"
> 
> #define ARRAY_SIZE(array) (sizeof (array) / sizeof *(array))
> 
> static int get_reg_list(int vcpu, struct kvm_reg_list **pregs, size_t *psize)
> {
> 	struct kvm_reg_list *regs = *pregs;
> 	size_t num_needed = 0;
> 	const size_t reg_offset = (char *)&regs->reg - (char *)regs;
> 
> 	while (1) {
> 		size_t size_needed = num_needed * sizeof regs->reg[0]
> 					+ reg_offset;
> 		if (size_needed > *psize) {
> 			regs = realloc(regs, size_needed);
> 			if (!regs)
> 				goto nomem;
> 
> 			*pregs = regs;
> 			*psize = size_needed;
> 		}
> 
> 		regs->n = (*psize - reg_offset) / sizeof regs->reg[0];
> 		if (!ioctl(vcpu, KVM_GET_REG_LIST, regs))
> 			break;
> 
> 		if (errno != E2BIG) {
> 			perror("KVM_GET_REG_LIST");
> 			goto error;
> 		}
> 
> 		num_needed = regs->n;
> 	}
> 
> 	return 0;	
> 
> nomem:
> 	errno = ENOMEM;
> 	perror(NULL);
> error:
> 	return -1;
> }
> 
> int main(void)
> {
> 	int fail = 0;
> 	int kvm_fd, vm, vcpu;
> 	struct kvm_vcpu_init init;
> 	size_t i;
> 	struct kvm_reg_list *regs = NULL;
> 	size_t regs_size = 0;
> 	uint64_t regdata[2];
> 	struct kvm_one_reg reg;
> 
> 	kvm_fd = open(KVM_PATH, O_RDWR);
> 	if (kvm_fd == -1) {
> 		perror(KVM_PATH);
> 		goto error;
> 	}
> 
> 	vm = ioctl(kvm_fd, KVM_CREATE_VM, 0);
> 	if (vm == -1) {
> 		perror("KVM_CREATE_VM");
> 		goto error;
> 	}
> 
> 	if (ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init)) {
> 		perror("KVM_ARM_PREFERRED_TARGET");
> 		goto error;
> 	}
> 
> 	printf("KVM_ARM_PREFERRED_TARGET:\n"
> 	       "\ttarget = %lu\n",
> 	       (unsigned long)init.target);
> 	for (i = 0; i < ARRAY_SIZE(init.features); ++i)
> 		printf("\tfeatures[0] = 0x%.8lx\n",
> 		       (unsigned long)init.features[i]);
> 	
> 	vcpu = ioctl(vm, KVM_CREATE_VCPU, 0);
> 	if (vcpu == -1) {
> 		perror("KVM_CREATE_VCPU");
> 		goto error;
> 	}
> 
> 	if (!get_reg_list(vcpu, &regs, &regs_size)) {
> 		fputs("Strange, KVM_GET_REG_LIST succeeded before KVM_VCPU_INIT!\n",
> 		      stderr);
> 			
> 		goto error;
> 	}
> 
> 	if (ioctl(vcpu, KVM_ARM_VCPU_INIT, &init)) {
> 		perror("KVM_ARM_VCPU_INIT");
> 		goto error;
> 	}
> 
> 	if (get_reg_list(vcpu, &regs, &regs_size))
> 		goto error;
> 
> 	assert(1 <= -1ULL >> 32);
> 	assert(ULONG_MAX >= (1ULL << 32) - 1);
> 	for (i = 0; i < regs->n; ++i) {
> 		size_t size;
> 		char const *s1 = "OK", *s2 = "", *s3 = "";;
> 
> 		size = (size_t)1 << ((regs->reg[i] & KVM_REG_SIZE_MASK) >>
> 				     KVM_REG_SIZE_SHIFT);
> 		if (size < 4 || size > 16) {
> 			s1 = "BADSIZE";
> 			fail = 1;
> 		} else {
> 			memset(&reg, 0, sizeof reg);
> 
> 			reg.id = regs->reg[i];
> 			reg.addr = (__u64)&regdata;
> 			if (ioctl(vcpu, KVM_GET_ONE_REG, &reg)) {
> 				s1 = "FAIL (";
> 				s2 = strerror(errno);
> 				s3 = ")";
> 				fail = 1;
> 			}
> 		}
> 
> 		printf("\t0x%.16lx\t%s%s%s\n",
> 		       (unsigned long)reg.id, s1, s2, s3);
> 	
> 	}
> 
> 	if (ferror(stdout) || fflush(stdout))
> 		goto stdout_error;
> 	
> 	if (fail) {
> 		puts("FAIL");
> 		goto error;
> 	}
> 
> 	if (puts("\nPASS") == EOF || fclose(stdout))
> 		goto stdout_error;
> 
> 	return EXIT_SUCCESS;
> 
> stdout_error:
> 	fputs("I/O error", stderr);
> error:
> 	return EXIT_FAILURE;
> }
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Dave Martin]

On Fri, Dec 14, 2018 at 03:25:45PM +0100, Andrew Jones wrote:
> 
> Hi Dave,
> 
> The below looks like a nice kvm selftest. Care to fit it into that
> framework (tools/testing/selftests/kvm/) and post it?

Sure, it makes sense not to drop this on the floor.  Do you feel the
test needs to go in alongside the fix, or is it alright if I repost this
later on?

It will need a bit of cleanup, and I'm juggling other stuff too...

Cheers
---Dave

> 
> Thanks,
> drew
> 
> > [1]
> > 
> > #include <assert.h>
> > #include <errno.h>
> > #include <limits.h>
> > #include <stddef.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <stdint.h>
> > #include <string.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <linux/kvm.h>
> > 
> > #define KVM_PATH "/dev/kvm"
> > 
> > #define ARRAY_SIZE(array) (sizeof (array) / sizeof *(array))
> > 
> > static int get_reg_list(int vcpu, struct kvm_reg_list **pregs, size_t *psize)
> > {
> > 	struct kvm_reg_list *regs = *pregs;
> > 	size_t num_needed = 0;
> > 	const size_t reg_offset = (char *)&regs->reg - (char *)regs;
> > 
> > 	while (1) {
> > 		size_t size_needed = num_needed * sizeof regs->reg[0]
> > 					+ reg_offset;
> > 		if (size_needed > *psize) {
> > 			regs = realloc(regs, size_needed);
> > 			if (!regs)
> > 				goto nomem;
> > 
> > 			*pregs = regs;
> > 			*psize = size_needed;
> > 		}
> > 
> > 		regs->n = (*psize - reg_offset) / sizeof regs->reg[0];
> > 		if (!ioctl(vcpu, KVM_GET_REG_LIST, regs))
> > 			break;
> > 
> > 		if (errno != E2BIG) {
> > 			perror("KVM_GET_REG_LIST");
> > 			goto error;
> > 		}
> > 
> > 		num_needed = regs->n;
> > 	}
> > 
> > 	return 0;	
> > 
> > nomem:
> > 	errno = ENOMEM;
> > 	perror(NULL);
> > error:
> > 	return -1;
> > }
> > 
> > int main(void)
> > {
> > 	int fail = 0;
> > 	int kvm_fd, vm, vcpu;
> > 	struct kvm_vcpu_init init;
> > 	size_t i;
> > 	struct kvm_reg_list *regs = NULL;
> > 	size_t regs_size = 0;
> > 	uint64_t regdata[2];
> > 	struct kvm_one_reg reg;
> > 
> > 	kvm_fd = open(KVM_PATH, O_RDWR);
> > 	if (kvm_fd == -1) {
> > 		perror(KVM_PATH);
> > 		goto error;
> > 	}
> > 
> > 	vm = ioctl(kvm_fd, KVM_CREATE_VM, 0);
> > 	if (vm == -1) {
> > 		perror("KVM_CREATE_VM");
> > 		goto error;
> > 	}
> > 
> > 	if (ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init)) {
> > 		perror("KVM_ARM_PREFERRED_TARGET");
> > 		goto error;
> > 	}
> > 
> > 	printf("KVM_ARM_PREFERRED_TARGET:\n"
> > 	       "\ttarget = %lu\n",
> > 	       (unsigned long)init.target);
> > 	for (i = 0; i < ARRAY_SIZE(init.features); ++i)
> > 		printf("\tfeatures[0] = 0x%.8lx\n",
> > 		       (unsigned long)init.features[i]);
> > 	
> > 	vcpu = ioctl(vm, KVM_CREATE_VCPU, 0);
> > 	if (vcpu == -1) {
> > 		perror("KVM_CREATE_VCPU");
> > 		goto error;
> > 	}
> > 
> > 	if (!get_reg_list(vcpu, &regs, &regs_size)) {
> > 		fputs("Strange, KVM_GET_REG_LIST succeeded before KVM_VCPU_INIT!\n",
> > 		      stderr);
> > 			
> > 		goto error;
> > 	}
> > 
> > 	if (ioctl(vcpu, KVM_ARM_VCPU_INIT, &init)) {
> > 		perror("KVM_ARM_VCPU_INIT");
> > 		goto error;
> > 	}
> > 
> > 	if (get_reg_list(vcpu, &regs, &regs_size))
> > 		goto error;
> > 
> > 	assert(1 <= -1ULL >> 32);
> > 	assert(ULONG_MAX >= (1ULL << 32) - 1);
> > 	for (i = 0; i < regs->n; ++i) {
> > 		size_t size;
> > 		char const *s1 = "OK", *s2 = "", *s3 = "";;
> > 
> > 		size = (size_t)1 << ((regs->reg[i] & KVM_REG_SIZE_MASK) >>
> > 				     KVM_REG_SIZE_SHIFT);
> > 		if (size < 4 || size > 16) {
> > 			s1 = "BADSIZE";
> > 			fail = 1;
> > 		} else {
> > 			memset(&reg, 0, sizeof reg);
> > 
> > 			reg.id = regs->reg[i];
> > 			reg.addr = (__u64)&regdata;
> > 			if (ioctl(vcpu, KVM_GET_ONE_REG, &reg)) {
> > 				s1 = "FAIL (";
> > 				s2 = strerror(errno);
> > 				s3 = ")";
> > 				fail = 1;
> > 			}
> > 		}
> > 
> > 		printf("\t0x%.16lx\t%s%s%s\n",
> > 		       (unsigned long)reg.id, s1, s2, s3);
> > 	
> > 	}
> > 
> > 	if (ferror(stdout) || fflush(stdout))
> > 		goto stdout_error;
> > 	
> > 	if (fail) {
> > 		puts("FAIL");
> > 		goto error;
> > 	}
> > 
> > 	if (puts("\nPASS") == EOF || fclose(stdout))
> > 		goto stdout_error;
> > 
> > 	return EXIT_SUCCESS;
> > 
> > stdout_error:
> > 	fputs("I/O error", stderr);
> > error:
> > 	return EXIT_FAILURE;
> > }
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Andrew Jones]

On Fri, Dec 14, 2018 at 02:41:21PM +0000, Dave Martin wrote:
> On Fri, Dec 14, 2018 at 03:25:45PM +0100, Andrew Jones wrote:
> > 
> > Hi Dave,
> > 
> > The below looks like a nice kvm selftest. Care to fit it into that
> > framework (tools/testing/selftests/kvm/) and post it?
> 
> Sure, it makes sense not to drop this on the floor.  Do you feel the
> test needs to go in alongside the fix, or is it alright if I repost this
> later on?

I'm fine with it getting posted whenever. I'll just be happy that it gets
posted :-)

Thanks,
drew

> 
> It will need a bit of cleanup, and I'm juggling other stuff too...
> 
> Cheers
> ---Dave
> 
> > 
> > Thanks,
> > drew
> > 
> > > [1]
> > > 
> > > #include <assert.h>
> > > #include <errno.h>
> > > #include <limits.h>
> > > #include <stddef.h>
> > > #include <stdio.h>
> > > #include <stdlib.h>
> > > #include <stdint.h>
> > > #include <string.h>
> > > #include <fcntl.h>
> > > #include <sys/ioctl.h>
> > > #include <linux/kvm.h>
> > > 
> > > #define KVM_PATH "/dev/kvm"
> > > 
> > > #define ARRAY_SIZE(array) (sizeof (array) / sizeof *(array))
> > > 
> > > static int get_reg_list(int vcpu, struct kvm_reg_list **pregs, size_t *psize)
> > > {
> > > 	struct kvm_reg_list *regs = *pregs;
> > > 	size_t num_needed = 0;
> > > 	const size_t reg_offset = (char *)&regs->reg - (char *)regs;
> > > 
> > > 	while (1) {
> > > 		size_t size_needed = num_needed * sizeof regs->reg[0]
> > > 					+ reg_offset;
> > > 		if (size_needed > *psize) {
> > > 			regs = realloc(regs, size_needed);
> > > 			if (!regs)
> > > 				goto nomem;
> > > 
> > > 			*pregs = regs;
> > > 			*psize = size_needed;
> > > 		}
> > > 
> > > 		regs->n = (*psize - reg_offset) / sizeof regs->reg[0];
> > > 		if (!ioctl(vcpu, KVM_GET_REG_LIST, regs))
> > > 			break;
> > > 
> > > 		if (errno != E2BIG) {
> > > 			perror("KVM_GET_REG_LIST");
> > > 			goto error;
> > > 		}
> > > 
> > > 		num_needed = regs->n;
> > > 	}
> > > 
> > > 	return 0;	
> > > 
> > > nomem:
> > > 	errno = ENOMEM;
> > > 	perror(NULL);
> > > error:
> > > 	return -1;
> > > }
> > > 
> > > int main(void)
> > > {
> > > 	int fail = 0;
> > > 	int kvm_fd, vm, vcpu;
> > > 	struct kvm_vcpu_init init;
> > > 	size_t i;
> > > 	struct kvm_reg_list *regs = NULL;
> > > 	size_t regs_size = 0;
> > > 	uint64_t regdata[2];
> > > 	struct kvm_one_reg reg;
> > > 
> > > 	kvm_fd = open(KVM_PATH, O_RDWR);
> > > 	if (kvm_fd == -1) {
> > > 		perror(KVM_PATH);
> > > 		goto error;
> > > 	}
> > > 
> > > 	vm = ioctl(kvm_fd, KVM_CREATE_VM, 0);
> > > 	if (vm == -1) {
> > > 		perror("KVM_CREATE_VM");
> > > 		goto error;
> > > 	}
> > > 
> > > 	if (ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init)) {
> > > 		perror("KVM_ARM_PREFERRED_TARGET");
> > > 		goto error;
> > > 	}
> > > 
> > > 	printf("KVM_ARM_PREFERRED_TARGET:\n"
> > > 	       "\ttarget = %lu\n",
> > > 	       (unsigned long)init.target);
> > > 	for (i = 0; i < ARRAY_SIZE(init.features); ++i)
> > > 		printf("\tfeatures[0] = 0x%.8lx\n",
> > > 		       (unsigned long)init.features[i]);
> > > 	
> > > 	vcpu = ioctl(vm, KVM_CREATE_VCPU, 0);
> > > 	if (vcpu == -1) {
> > > 		perror("KVM_CREATE_VCPU");
> > > 		goto error;
> > > 	}
> > > 
> > > 	if (!get_reg_list(vcpu, &regs, &regs_size)) {
> > > 		fputs("Strange, KVM_GET_REG_LIST succeeded before KVM_VCPU_INIT!\n",
> > > 		      stderr);
> > > 			
> > > 		goto error;
> > > 	}
> > > 
> > > 	if (ioctl(vcpu, KVM_ARM_VCPU_INIT, &init)) {
> > > 		perror("KVM_ARM_VCPU_INIT");
> > > 		goto error;
> > > 	}
> > > 
> > > 	if (get_reg_list(vcpu, &regs, &regs_size))
> > > 		goto error;
> > > 
> > > 	assert(1 <= -1ULL >> 32);
> > > 	assert(ULONG_MAX >= (1ULL << 32) - 1);
> > > 	for (i = 0; i < regs->n; ++i) {
> > > 		size_t size;
> > > 		char const *s1 = "OK", *s2 = "", *s3 = "";;
> > > 
> > > 		size = (size_t)1 << ((regs->reg[i] & KVM_REG_SIZE_MASK) >>
> > > 				     KVM_REG_SIZE_SHIFT);
> > > 		if (size < 4 || size > 16) {
> > > 			s1 = "BADSIZE";
> > > 			fail = 1;
> > > 		} else {
> > > 			memset(&reg, 0, sizeof reg);
> > > 
> > > 			reg.id = regs->reg[i];
> > > 			reg.addr = (__u64)&regdata;
> > > 			if (ioctl(vcpu, KVM_GET_ONE_REG, &reg)) {
> > > 				s1 = "FAIL (";
> > > 				s2 = strerror(errno);
> > > 				s3 = ")";
> > > 				fail = 1;
> > > 			}
> > > 		}
> > > 
> > > 		printf("\t0x%.16lx\t%s%s%s\n",
> > > 		       (unsigned long)reg.id, s1, s2, s3);
> > > 	
> > > 	}
> > > 
> > > 	if (ferror(stdout) || fflush(stdout))
> > > 		goto stdout_error;
> > > 	
> > > 	if (fail) {
> > > 		puts("FAIL");
> > > 		goto error;
> > > 	}
> > > 
> > > 	if (puts("\nPASS") == EOF || fclose(stdout))
> > > 		goto stdout_error;
> > > 
> > > 	return EXIT_SUCCESS;
> > > 
> > > stdout_error:
> > > 	fputs("I/O error", stderr);
> > > error:
> > > 	return EXIT_FAILURE;
> > > }
> > > _______________________________________________
> > > kvmarm mailing list
> > > kvmarm@lists.cs.columbia.edu
> > > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Marc Zyngier]

On 12/12/2018 20:17, Dave Martin wrote:
> Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
> access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
> that do not correspond to a single underlying architectural register.
> 
> KVM_GET_REG_LIST was not changed to match however: instead, it
> simply yields a list of 32-bit register IDs that together cover the
> whole kvm_regs struct.  This means that if userspace tries to use
> the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
> some of those calls will now fail.
> 
> This was not the intention.  Instead, iterating KVM_*_ONE_REG over
> the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
> to work.
> 
> This patch fixes the problem by splitting validate_core_reg_id()
> into a backend core_reg_size_from_offset() which does all of the
> work except for checking that the size field in the register ID
> matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
> converted to use this to enumerate the valid offsets.
> 
> kvm_arm_copy_reg_indices() now also sets the register ID size field
> appropriately based on the value returned, so the register ID
> supplied to userspace is fully qualified for use with the register
> access ioctls.
> 
> Cc: stable@vger.kernel.org
> Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from userspace")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/guest.c | 61 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index dd436a5..cbe423b 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -57,9 +57,8 @@ static u64 core_reg_offset_from_id(u64 id)
>  	return id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_CORE);
>  }
>  
> -static int validate_core_offset(const struct kvm_one_reg *reg)
> +static int core_reg_size_from_offset(u64 off)
>  {
> -	u64 off = core_reg_offset_from_id(reg->id);
>  	int size;
>  
>  	switch (off) {
> @@ -89,8 +88,21 @@ static int validate_core_offset(const struct kvm_one_reg *reg)
>  		return -EINVAL;
>  	}
>  
> -	if (KVM_REG_SIZE(reg->id) == size &&
> -	    IS_ALIGNED(off, size / sizeof(__u32)))
> +	if (IS_ALIGNED(off, size / sizeof(__u32)))
> +		return size;
> +
> +	return -EINVAL;
> +}
> +
> +static int validate_core_offset(const struct kvm_one_reg *reg)
> +{
> +	u64 off = core_reg_offset_from_id(reg->id);
> +	int size = core_reg_size_from_offset(off);
> +
> +	if (size < 0)
> +		return -EINVAL;
> +
> +	if (KVM_REG_SIZE(reg->id) == size)
>  		return 0;
>  
>  	return -EINVAL;
> @@ -195,7 +207,19 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  
>  static unsigned long num_core_regs(void)
>  {
> -	return sizeof(struct kvm_regs) / sizeof(__u32);
> +	unsigned int i;
> +	int n = 0;
> +
> +	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
> +		int size = core_reg_size_from_offset(i);
> +
> +		if (size < 0)
> +			continue;
> +
> +		n++;
> +	}
> +
> +	return n;
>  }
>  
>  /**
> @@ -270,11 +294,34 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
>  int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  {
>  	unsigned int i;
> -	const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE;
>  	int ret;
>  
>  	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
> -		if (put_user(core_reg | i, uindices))
> +		u64 reg = KVM_REG_ARM64 | KVM_REG_ARM_CORE | i;
> +		int size = core_reg_size_from_offset(i);
> +
> +		if (size < 0)
> +			continue;
> +
> +		switch (size) {
> +		case sizeof(__u32):
> +			reg |= KVM_REG_SIZE_U32;
> +			break;
> +
> +		case sizeof(__u64):
> +			reg |= KVM_REG_SIZE_U64;
> +			break;
> +
> +		case sizeof(__uint128_t):
> +			reg |= KVM_REG_SIZE_U128;
> +			break;
> +
> +		default:
> +			WARN_ON(1);
> +			continue;
> +		}
> +
> +		if (put_user(reg, uindices))
>  			return -EFAULT;
>  		uindices++;
>  	}
> 

I'm quite keen on queuing this for 4.21, but I'd like Peter's seal of
approval on it.

Peter?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] KVM: arm64: Filter out invalid core register IDs in KVM_GET_REG_LIST

[Peter Maydell]

On Tue, 18 Dec 2018 at 14:00, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 12/12/2018 20:17, Dave Martin wrote:
> > Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
> > access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
> > that do not correspond to a single underlying architectural register.
> >
> > KVM_GET_REG_LIST was not changed to match however: instead, it
> > simply yields a list of 32-bit register IDs that together cover the
> > whole kvm_regs struct.  This means that if userspace tries to use
> > the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
> > some of those calls will now fail.
> >
> > This was not the intention.  Instead, iterating KVM_*_ONE_REG over
> > the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
> > to work.
> >
> > This patch fixes the problem by splitting validate_core_reg_id()
> > into a backend core_reg_size_from_offset() which does all of the
> > work except for checking that the size field in the register ID
> > matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
> > converted to use this to enumerate the valid offsets.
> >
> > kvm_arm_copy_reg_indices() now also sets the register ID size field
> > appropriately based on the value returned, so the register ID
> > supplied to userspace is fully qualified for use with the register
> > access ioctls.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from userspace")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---

> I'm quite keen on queuing this for 4.21, but I'd like Peter's seal of
> approval on it.
>
> Peter?

Sounds plausible, but I'm on holiday now til January, so if you
want a more detailed response than that (or even actual testing
with QEMU) it'll have to wait til next year. (Note that I never
tested d26c25a9d19b with QEMU either...)

thanks
-- PMM

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] KVM: arm64: Factor out KVM_GET_REG_LIST core register enumeration

[Dave Martin]

Currently, enumeration of the core register IDs for
KVM_GET_REG_LIST is open-coded in kvm_arm_copy_reg_indices().

This will become cumbersome as the enumeration logic becomes more
complex.  In preparation for future patches, this patch factors the
code out into a separate function copy_core_reg_indices(),
consistently with the way other classes of registers are handled.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 65 ++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index cbe423b..46eb867 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -205,23 +205,53 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 	return -EINVAL;
 }
 
-static unsigned long num_core_regs(void)
+static int copy_core_reg_indices(u64 __user **uind)
 {
 	unsigned int i;
 	int n = 0;
 
 	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
+		u64 reg = KVM_REG_ARM64 | KVM_REG_ARM_CORE | i;
 		int size = core_reg_size_from_offset(i);
 
 		if (size < 0)
 			continue;
 
+		switch (size) {
+		case sizeof(__u32):
+			reg |= KVM_REG_SIZE_U32;
+			break;
+
+		case sizeof(__u64):
+			reg |= KVM_REG_SIZE_U64;
+			break;
+
+		case sizeof(__uint128_t):
+			reg |= KVM_REG_SIZE_U128;
+			break;
+
+		default:
+			WARN_ON(1);
+			continue;
+		}
+
+		if (uind) {
+			if (put_user(reg, *uind))
+				return -EFAULT;
+			++*uind;
+		}
+
 		n++;
 	}
 
 	return n;
 }
 
+static unsigned long num_core_regs(void)
+{
+	return copy_core_reg_indices(NULL);
+}
+
 /**
  * ARM64 versions of the TIMER registers, always available on arm64
  */
@@ -293,38 +323,11 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
  */
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
-	unsigned int i;
 	int ret;
 
-	for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
-		u64 reg = KVM_REG_ARM64 | KVM_REG_ARM_CORE | i;
-		int size = core_reg_size_from_offset(i);
-
-		if (size < 0)
-			continue;
-
-		switch (size) {
-		case sizeof(__u32):
-			reg |= KVM_REG_SIZE_U32;
-			break;
-
-		case sizeof(__u64):
-			reg |= KVM_REG_SIZE_U64;
-			break;
-
-		case sizeof(__uint128_t):
-			reg |= KVM_REG_SIZE_U128;
-			break;
-
-		default:
-			WARN_ON(1);
-			continue;
-		}
-
-		if (put_user(reg, uindices))
-			return -EFAULT;
-		uindices++;
-	}
+	ret = copy_core_reg_indices(&uindices);
+	if (ret < 0)
+		return ret;
 
 	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
 	if (ret)
-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel