Re: [PATCH v5 29/51] x86/kvm: Don't disable kvmclock on BSP in syscore_suspend()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch inadvertently leaves the BSP's kvmclock enabled when powering off the machine for ACPI S4 hibernation, leading to potential memory corruption when the VM awakens.

Pre-existing issues:
- [Critical] Stack-allocated waitqueue head use-after-free in async page fault handling.
--

Note: The standard inline diff format has been altered to a free-form
summary due to patch recitation restrictions.

commit 03858970f2d7a88b91bdd5b16c1573166126d3c4
Author: Sean Christopherson <seanjc@google.com>
Subject: x86/kvm: Don't disable kvmclock on BSP in syscore_suspend()

This patch modifies the guest suspend logic so that kvmclock is not disabled
on the BSP during syscore_suspend(), relying instead on the sched_clock
restore callback.

[Severity: High]
Does this change inadvertently leave the BSP's kvmclock enabled when
powering off the machine for ACPI S4 hibernation?

Looking at the final S4 power-off path in hibernation_platform_enter() in
kernel/power/hibernate.c, it calls syscore_suspend() but does not call
save_processor_state(). Because this patch removes the unconditional BSP
kvmclock_disable() from syscore_suspend() and relies on
kvm_save_sched_clock_state() (which only fires during image creation),
kvmclock appears to remain fully enabled during the S4 power-off.

Could this lead to memory corruption when the VM awakens, since the
hypervisor might continue writing to the old kvmclock address?

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a use-after-free
risk with the stack-allocated waitqueue head in the async page fault
handling in arch/x86/kernel/kvm.c?

When a guest task waits for an async page fault in
kvm_async_pf_task_wait_schedule(), it waits on a stack-allocated struct
kvm_task_sleep_node. Concurrently, when a waker CPU executes
apf_task_wake_one(), it unhashes the node via hlist_del_init().

The waiter checks hlist_unhashed() locklessly. If it observes the unhashed
state before the waker is finished, the waiter can break its loop, return,
and destroy the stack frame. The waker might then dereference the
deallocated waitqueue head to call swake_up_one(), potentially corrupting
stack memory.

Can this sequence occur during concurrent page fault completions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701193212.749551-1-seanjc@google.com?part=29