On 11.03.2025 22:10, Andrew Cooper wrote:
> _show_registers() prints the data selectors from struct cpu_user_regs, but
> these fields are sometimes out-of-bounds. See commit 6065a05adf15
> ("x86/traps: 'Fix' safety of read_registers() in #DF path").
>
> There are 3 callers of _show_registers():
>
> 1. vcpu_show_registers(), which always operates on a scheduled-out vCPU,
> where v->arch.user_regs (or aux_regs on the stack) is always in-bounds.
>
> 2. show_registers() where regs is always an on-stack frame. regs is copied
> into a local variable first (which is an OoB read for constructs such as
> WARN()), before being modified (so no OoB write).
>
> 3. do_double_fault(), where regs is adjacent to the stack guard page, and
> written into directly. This is an out of bounds read and write, with a
> bodge to avoid the writes hitting the guard page.
>
> Include the data segment selectors in struct extra_state, and use those fields
> instead of the fields in regs. This resolves the OoB write on the #DF path.
>
> Resolve the OoB read in show_registers() by doing a partial memcpy() rather
> than full structure copy. This is temporary until we've finished untangling
> the vm86 fields fully.
>
> Signed-off-by: Andrew Cooper <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>
> @@ -124,17 +128,23 @@ static void _show_registers(
> state->fsb, state->gsb, state->gss);
> printk("ds: %04x es: %04x fs: %04x gs: %04x "
> "ss: %04x cs: %04x\n",
> - regs->ds, regs->es, regs->fs,
> - regs->gs, regs->ss, regs->cs);
> + state->ds, state->es, state->fs,
> + state->gs, regs->ss, regs->cs);
> }
>
> void show_registers(const struct cpu_user_regs *regs)
> {
> - struct cpu_user_regs fault_regs = *regs;
> + struct cpu_user_regs fault_regs;
> struct extra_state fault_state;
> enum context context;
> struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
>
> + /*
> + * Don't read beyond the end of the hardware frame. It is out of bounds
> + * for WARN()/etc.
> + */
> + memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
I don't like this (especially the assumption on es being special, much like
e.g. get_stack_bottom() also does) very much, but I hope this is going to
disappear at some point anyway.
Jan
