[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 17 Mar 2025 12:00:53 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 17 Mar 2025 11:01:04 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.