Re: [PATCH v3 18/41] x86/paravirt: Pass sched_clock save/restore helpers during registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference in ACPI suspend/resume and kexec paths due to nullified sched_clock save/restore hooks for VMware and Xen guests.
--

commit 812ddcb2484da2fae1600ef7146674adbef2e760
Author: Sean Christopherson <seanjc@google.com>

x86/paravirt: Pass sched_clock save/restore helpers during registration
    
Pass in a PV clock's save/restore helpers when configuring sched_clock
instead of relying on each PV clock to manually set the save/restore hooks.
In addition to bringing sanity to the code, this will allow gracefully
"rejecting" a PV sched_clock, e.g. when running as a CoCo guest that has
access to a "secure" TSC.
    
No functional change intended.

> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index b5cb66ca022b8..968de002975f9 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -347,11 +347,8 @@ static void __init vmware_paravirt_ops_setup(void)
>  
>  	vmware_cyc2ns_setup();
>  
> -	if (vmw_sched_clock) {
> -		paravirt_set_sched_clock(vmware_sched_clock);
> -		x86_platform.save_sched_clock_state = NULL;
> -		x86_platform.restore_sched_clock_state = NULL;
> -	}
> +	if (vmw_sched_clock)
> +		paravirt_set_sched_clock(vmware_sched_clock, NULL, NULL);

[ ... ]

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index b14c4ada89a3f..0114c63dfdd9a 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -280,12 +280,15 @@ bool using_native_sched_clock(void)
>  	return static_call_query(pv_sched_clock) == native_sched_clock;
>  }
>  
> -void __paravirt_set_sched_clock(u64 (*func)(void), bool stable)
> +void __paravirt_set_sched_clock(u64 (*func)(void), bool stable,
> +				void (*save)(void), void (*restore)(void))
>  {
>  	if (!stable)
>  		clear_sched_clock_stable();
>  
>  	static_call_update(pv_sched_clock, func);
> +	x86_platform.save_sched_clock_state = save;
> +	x86_platform.restore_sched_clock_state = restore;

Does assigning these function pointers without a null check cause a null
pointer dereference during ACPI suspend/resume or kexec paths for VMware
and Xen guests?

When VMware and Xen pass NULL for these hooks (as formalized by this patch
and introduced in an earlier patch in this series), they are stored directly
in x86_platform.

Then, unconditionally in arch/x86/power/cpu.c:save_processor_state():

void save_processor_state(void)
{
	__save_processor_state(&saved_context);
	x86_platform.save_sched_clock_state();
}

and similarly in __restore_processor_state():

	x86_platform.restore_sched_clock_state();

Will these calls jump to a null pointer since the hooks are now NULL?

Before this patch series, these platforms relied on the default
tsc_save_sched_clock_state hooks, which safely returned early. Setting the
hooks to NULL appears to convert this safe no-op into a regression that
triggers a panic.

Should __paravirt_set_sched_clock() substitute a dummy no-op function when
NULL is passed, or should the caller sites in arch/x86/power/cpu.c explicitly
check if the hooks are set before invocation?

>  }

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