[PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[Alexey Makhalov]

Caller of vmware_hypercall_slow() can pass NULL into *out1,
*out2,... *out5. It will lead to a NULL pointer dereference.

Check a pointer for NULL before assigning a value.

Fixes: 	666cbb562d05d ("x86/vmware: Introduce VMware hypercall API")
Co-developed-by: Alex James <alex.james@broadcom.com>
Signed-off-by: Alex James <alex.james@broadcom.com>
Signed-off-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
---
 arch/x86/kernel/cpu/vmware.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 55903563afd3..16da970499f2 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -72,13 +72,13 @@ unsigned long vmware_hypercall_slow(unsigned long cmd,
 				    u32 *out1, u32 *out2, u32 *out3,
 				    u32 *out4, u32 *out5)
 {
-	unsigned long out0;
+	unsigned long out0, rbx, rcx, rdx, rsi, rdi;
 
 	switch (vmware_hypercall_mode) {
 	case CPUID_VMWARE_FEATURES_ECX_VMCALL:
 		asm_inline volatile ("vmcall"
-				: "=a" (out0), "=b" (*out1), "=c" (*out2),
-				"=d" (*out3), "=S" (*out4), "=D" (*out5)
+				: "=a" (out0), "=b" (rbx), "=c" (rcx),
+				"=d" (rdx), "=S" (rsi), "=D" (rdi)
 				: "a" (VMWARE_HYPERVISOR_MAGIC),
 				"b" (in1),
 				"c" (cmd),
@@ -89,8 +89,8 @@ unsigned long vmware_hypercall_slow(unsigned long cmd,
 		break;
 	case CPUID_VMWARE_FEATURES_ECX_VMMCALL:
 		asm_inline volatile ("vmmcall"
-				: "=a" (out0), "=b" (*out1), "=c" (*out2),
-				"=d" (*out3), "=S" (*out4), "=D" (*out5)
+				: "=a" (out0), "=b" (rbx), "=c" (rcx),
+				"=d" (rdx), "=S" (rsi), "=D" (rdi)
 				: "a" (VMWARE_HYPERVISOR_MAGIC),
 				"b" (in1),
 				"c" (cmd),
@@ -101,8 +101,8 @@ unsigned long vmware_hypercall_slow(unsigned long cmd,
 		break;
 	default:
 		asm_inline volatile ("movw %[port], %%dx; inl (%%dx), %%eax"
-				: "=a" (out0), "=b" (*out1), "=c" (*out2),
-				"=d" (*out3), "=S" (*out4), "=D" (*out5)
+				: "=a" (out0), "=b" (rbx), "=c" (rcx),
+				"=d" (rdx), "=S" (rsi), "=D" (rdi)
 				: [port] "i" (VMWARE_HYPERVISOR_PORT),
 				"a" (VMWARE_HYPERVISOR_MAGIC),
 				"b" (in1),
@@ -113,6 +113,18 @@ unsigned long vmware_hypercall_slow(unsigned long cmd,
 				: "cc", "memory");
 		break;
 	}
+
+	if (out1)
+		*out1 = rbx;
+	if (out2)
+		*out2 = rcx;
+	if (out3)
+		*out3 = rdx;
+	if (out4)
+		*out4 = rsi;
+	if (out5)
+		*out5 = rdi;
+
 	return out0;
 }
 
-- 
2.39.4

Re: [PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[Borislav Petkov]

On Tue, Jun 25, 2024 at 01:33:48AM -0700, Alexey Makhalov wrote:
> Caller of vmware_hypercall_slow() can pass NULL into *out1,
> *out2,... *out5. It will lead to a NULL pointer dereference.
> 
> Check a pointer for NULL before assigning a value.

I queue your patches and *now* you find this?!

How did you test them in the first place and why was this scenario missed?

Geez.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[Alexey Makhalov]

My test environment was screwed up during the last version of the 
patchset. I was using a kernel which was built previously and didn't pay 
attention to commit hash suffix in `uname -r`.
Human mistake, apologize for that.

Alex found it while doing TDX testing on x86/vmware on tip.

Do you want me to resubmit the patchset to do not brake a git bisect?

On 6/25/24 1:47 AM, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 01:33:48AM -0700, Alexey Makhalov wrote:
>> Caller of vmware_hypercall_slow() can pass NULL into *out1,
>> *out2,... *out5. It will lead to a NULL pointer dereference.
>>
>> Check a pointer for NULL before assigning a value.
> 
> I queue your patches and *now* you find this?!
> 
> How did you test them in the first place and why was this scenario missed?
> 
> Geez.
> 

Re: [PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[Borislav Petkov]

On Tue, Jun 25, 2024 at 07:45:50AM -0700, Alexey Makhalov wrote:
> My test environment was screwed up during the last version of the patchset.
> I was using a kernel which was built previously and didn't pay attention to
> commit hash suffix in `uname -r`.
> Human mistake, apologize for that.

Ok, no worries, happens.

> Alex found it while doing TDX testing on x86/vmware on tip.
> 
> Do you want me to resubmit the patchset to do not brake a git bisect?

Nah, I'll fold it into the broken patch and rebase.

Btw, pls do me a favor and do not top-post but put your reply underneath the
quoted text, like we all do.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/vmware: fix panic in vmware_hypercall_slow()

[Alexey Makhalov]

On 6/25/24 7:51 AM, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 07:45:50AM -0700, Alexey Makhalov wrote:
>> My test environment was screwed up during the last version of the patchset.
>> I was using a kernel which was built previously and didn't pay attention to
>> commit hash suffix in `uname -r`.
>> Human mistake, apologize for that.
> 
> Ok, no worries, happens.
> 
>> Alex found it while doing TDX testing on x86/vmware on tip.
>>
>> Do you want me to resubmit the patchset to do not brake a git bisect?
> 
> Nah, I'll fold it into the broken patch and rebase.

Thanks for your time.

> 
> Btw, pls do me a favor and do not top-post but put your reply underneath the
> quoted text, like we all do.
Noted.

> 
> Thx.
>