Re: [Devel] [PATCH RK VZ10] x86/traps: emulate CPUID faults for user tasks in exc_general_protection()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] x86/vdso: check_cpuid_fault() only matches bare 0f a2 and adds a fixed 2 to IP
- [Low] x86/vdso: misspelled label try_fixup_cupid in fixup_vdso_exception()
- [Low] x86/vdso: check_cpuid_fault() uses convert_ip_to_linear() and a bare get_user()
--

commit c9d27b496f26c56280e62cfc00914a978e22f21d
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

x86/traps: emulate CPUID faults for user tasks in exc_general_protection()

This commit adds a CPUID-fault emulation helper to
arch/x86/entry/vdso/extable.c and calls it from fixup_vdso_exception() on
the path where no vDSO fixup applies. The aim is for a user-mode #GP(0)
caused by CPUID to be emulated with the cpuid_override table instead of
ending in SIGSEGV.

Link: http://lore.virtuozzo.com/20261006-b4-prep-khorenko-vstor-148656-just-cpuid-v1-1-6f2a2a6deef3@virtuozzo.com

> diff --git a/arch/x86/entry/vdso/extable.c b/arch/x86/entry/vdso/extable.c
> index afcf5b65beef7..ca8c184925ec2 100644
> --- a/arch/x86/entry/vdso/extable.c
> +++ b/arch/x86/entry/vdso/extable.c
> @@ -9,6 +9,30 @@ struct vdso_exception_table_entry {
>  	int insn, fixup;
>  };
>  
> +static int check_cpuid_fault(struct pt_regs *regs, long error_code)
> +{
> +	unsigned long addr;
> +	unsigned short opcode;
> +
> +	if (error_code != 0)
> +		return 0;
> +
> +	if (test_thread_flag(TIF_NOCPUID))
> +		return 0;
> +
> +	addr = convert_ip_to_linear(current, regs);

[Severity: Low]
Can convert_ip_to_linear() give the wrong address here?

With CONFIG_MODIFY_LDT_SYSCALL, convert_ip_to_linear() adds
get_desc_base(desc) for an LDT code segment. It never checks the 64-bit (L)
bit, and in long mode the hardware ignores that base.

If a task installs an LDT code segment with a non-zero base via
modify_ldt() and takes a #GP(0), get_user() reads the opcode from ip+base
instead of ip. Two things can then happen:

- A non-CPUID fault is treated as CPUID. do_cpuid_fault() runs and
  regs->ip is advanced by 2, so the task survives a fault that should have
  raised SIGSEGV.
- A genuine CPUID is missed, and the task gets SIGSEGV instead of the
  override.

The existing check_cpuid_fault() in arch/x86/kernel/traps.c uses
insn_fetch_from_user() and insn_decode_from_regs(), which handle segment
and mode details. Could the new helper use the same approach?

> +	if (get_user(opcode, (unsigned short __user *)addr))
> +		return 0;
> +
> +	if (opcode != 0xa20f)
> +		return 0;

[Severity: Medium]
Does this check decline a CPUID that has a prefix?

The helper accepts only the bare two-byte encoding 0f a2. A prefixed
encoding such as 66 0f a2, 48 0f a2 or a legacy prefix fails the compare,
and check_cpuid_fault() returns 0.

The traps.c version decodes the instruction and checks both opcode bytes:

  if (insn.opcode.bytes[0] != 0x0f || insn.opcode.bytes[1] != 0xa2)
          return 0;
  do_cpuid_fault(regs);
  regs->ip += insn.length;

The commit message says the traps.c call is unreachable for user-mode #GP
on the livepatch target. If so, this helper is the only emulation path
there. A task with TIF_CPUID_OVERRIDE that runs a prefixed CPUID would then
reach gp_user_force_sig_segv(), which is the failure this patch is meant to
remove.

In the tree I reviewed, exc_general_protection() still calls the
decoder-based check_cpuid_fault() inside the user_mode() branch after
fixup_vdso_exception(), so prefixed CPUID would fall back to it there.
Compilers and glibc normally emit a bare CPUID, so this should be rare.

> +
> +	do_cpuid_fault(regs);
> +
> +	regs->ip += 2;

[Severity: Medium]
Is the fixed increment of 2 correct for every CPUID encoding?

The traps.c version advances ip by insn.length, so the two functions can
disagree. Could this helper decode the instruction and use the real length,
as the traps.c helper does?

> +	return 1;
> +}
> +
>  bool fixup_vdso_exception(struct pt_regs *regs, int trapnr,
>  			  unsigned long error_code, unsigned long fault_addr)
>  {

[ ... ]

> @@ -26,7 +50,7 @@ bool fixup_vdso_exception(struct pt_regs *regs, int trapnr,
>  		return false;
>  
>  	if (!current->mm->context.vdso)
> -		return false;
> +		goto try_fixup_cupid;
>  
> [ ... ]
> +try_fixup_cupid:

[Severity: Low]
This isn't a bug, but the label is misspelled: try_fixup_cupid should
presumably be try_fixup_cpuid.

> +	/* Only exc_general_protection can go this way */
> +	if (trapnr == X86_TRAP_GP && check_cpuid_fault(regs, error_code))
> +		return true;

This isn't a bug, but the comment "Only exc_general_protection can go
this way" describes a caller convention that the code doesn't enforce. The
only real gate is trapnr == X86_TRAP_GP.

> +
>  	return false;
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102100432.15867-1-vladimir.riabchun@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel