Re: [PATCH 3/8] x86/traps: Avoid OoB accesses to print the data selectors

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/2025 11:00 am, Jan Beulich wrote:
> 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 <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
>> @@ -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.

As noted, it's temporary, and goes away in patch 8.

~Andrew