Re: [PATCH 04/45] KVM: x86/emulate: Check TSS busy-bit writes during task switches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: x86/emulate: unhandled write_segment_descriptor() error in task switch
- [High] KVM: x86/emulate: task switch drops faults and IO_NEEDED on descriptor writes
--

commit 31629465ad13b50735702c0107b5866c7a5a5806
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86/emulate: Check TSS busy-bit writes during task switches

This commit checks the return values of the write_segment_descriptor() calls
that clear the old TSS busy bit and set the new TSS busy bit in
emulator_do_task_switch(), returning early on failure so that failed writes
stop the task switch.

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index c1b21282187f3..711794592c2ca 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2925,7 +2925,9 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  
>  	if (reason == TASK_SWITCH_IRET || reason == TASK_SWITCH_JMP) {
>  		curr_tss_desc.type &= ~(1 << 1); /* clear busy flag */
> -		write_segment_descriptor(ctxt, old_tss_sel, &curr_tss_desc);
> +		ret = write_segment_descriptor(ctxt, old_tss_sel, &curr_tss_desc);
> +		if (ret != X86EMUL_CONTINUE)
> +			return ret;

[Severity: High]
When write_segment_descriptor() fails in arch/x86/kvm/emulate.c:
emulator_do_task_switch(), will returning the non-X86EMUL_CONTINUE status
actually reach the emulation-failure exit in kvm_task_switch(), or does it
cause emulator_task_switch() to return EMULATION_OK while skipping register
writeback?

Tracing the call chain from write_segment_descriptor() ->
linear_write_system() -> emulator_write_std() ->
kvm_write_guest_virt_helper():

arch/x86/kvm/x86.c:kvm_write_guest_virt_helper() {
	...
		if (gpa == INVALID_GPA)
			return X86EMUL_PROPAGATE_FAULT;
		ret = kvm_vcpu_write_guest(vcpu, gpa, data, towrite);
		if (ret < 0) {
			r = X86EMUL_IO_NEEDED;
			goto out;
		}
	...
}

At this commit (prior to the later commit "KVM: x86/emulate: Propagate memory
faults to userspace" in the series), any write failure returns
X86EMUL_PROPAGATE_FAULT or X86EMUL_IO_NEEDED (without setting
vcpu->mmio_needed), and never returns X86EMUL_UNHANDLEABLE. Even after commit
"KVM: x86/emulate: Propagate memory faults to userspace" wires up
X86EMUL_MEMORY_FAULT to EMULATION_MEMORY_FAULT, non-memory-attribute failures
still return X86EMUL_PROPAGATE_FAULT (for example, when the GDT page is
mapped read-only in the guest page tables with CR0.WP=1) or
X86EMUL_IO_NEEDED (for example, when the GDT is in a read-only memslot).

In arch/x86/kvm/emulate.c:emulator_task_switch():

	if (rc == X86EMUL_CONTINUE) {
		ctxt->eip = ctxt->_eip;
		writeback_registers(ctxt);
	}

	return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;

And in arch/x86/kvm/x86.c:kvm_task_switch():

	if (ret || vcpu->mmio_needed)
		goto unhandled_task_switch;

	kvm_rip_write(vcpu, ctxt->eip);
	kvm_set_rflags(vcpu, ctxt->eflags);
	return 1;

When this first write_segment_descriptor() call fails on an IRET or JMP task
switch, skip_emulated_instruction() has already advanced RIP, and
emulator_task_switch() returns EMULATION_OK (0) with vcpu->mmio_needed false.
Doesn't kvm_task_switch() then silently turn the task switch into a no-op and
resume the guest without injecting an exception or exiting to userspace?

>  	}
>  
>  	if (reason == TASK_SWITCH_IRET)
> @@ -2949,7 +2951,9 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  
>  	if (reason != TASK_SWITCH_IRET) {
>  		next_tss_desc.type |= (1 << 1); /* set busy flag */
> -		write_segment_descriptor(ctxt, tss_selector, &next_tss_desc);
> +		ret = write_segment_descriptor(ctxt, tss_selector, &next_tss_desc);
> +		if (ret != X86EMUL_CONTINUE)
> +			return ret;

[Severity: High]
Similarly, if this second write_segment_descriptor() call in
arch/x86/kvm/emulate.c:emulator_do_task_switch() fails with
X86EMUL_PROPAGATE_FAULT or X86EMUL_IO_NEEDED (for instance, if the GDT page
containing next_tss_desc is read-only or unmapped under the new CR3 just
loaded by load_state_from_tss32() when segment descriptors are VM86/NULL or
already have the Accessed bit set, or if the GDT resides in a read-only
memslot), can returning early here leave the vCPU in a half-switched state?

At this point in emulator_do_task_switch(), task_switch_16() or
task_switch_32() has already committed the new task's CR3, ctxt->eflags, and
segment registers (LDTR, ES, CS, SS, DS, FS, GS). Returning early skips
setting X86_CR0_TS, loading VCPU_SREG_TR, pushing error_code, and updating
dr7 in emulator_do_task_switch(), and skips updating ctxt->eip = ctxt->_eip
and calling writeback_registers(ctxt) in emulator_task_switch().

Because emulator_task_switch() still translates X86EMUL_PROPAGATE_FAULT and
X86EMUL_IO_NEEDED into EMULATION_OK without setting ctxt->have_exception, and
arch/x86/kvm/x86.c:kvm_task_switch() neither checks ctxt->have_exception nor
takes the unhandled_task_switch path when ret is 0 and vcpu->mmio_needed is
false:

	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
				   has_error_code, error_code);

	/*
	 * Report an error userspace if MMIO is needed, as KVM doesn't support
	 * MMIO during a task switch (or any other complex operation).
	 */
	if (ret || vcpu->mmio_needed)
		goto unhandled_task_switch;

	kvm_rip_write(vcpu, ctxt->eip);
	kvm_set_rflags(vcpu, ctxt->eflags);
	return 1;

Won't kvm_task_switch() drop the architectural fault or write failure and
resume the guest with the new task's CR3, EFLAGS, and segments but the old
task's TR, RIP, and general-purpose registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174130.8779-1-pbonzini@redhat.com?part=4