Re: [PATCH 09/15] x86_64: fix exception stack alignment

[Samuel Thibault]

Luca Dariz, le mar. 28 juin 2022 12:10:48 +0200, a ecrit:
> * i386/i386/pcb.c:
>   - increase alignment of pcb cache to 16
>   - ensure the stack is properly aligned when switching ktss
> * i386/i386/thread.h:
>   - add padding tomake iss field end aligned to 16 bytes
> * i386/i386/trap.c:
>   - ensure the state we get after the trap points to the correct place
>     in the pcb structure
> 
> When handling exceptions from IA-32e compatibility mode in user space,
> on a 64-bit kernel, the exception stack where error info is pushed
> needs to be aligned to 16 bytes (see Intel System Programming guide,
> $6.14.2)
> 
> The exception stack frame is set in the middle of pcb->iss, but it's not 
> always
> 16-byte aligned; to make sure it is, we increase the alignment of the
> pcb cache and add a padding field in the pcb structure.
> 
> This issue resulted in a general protection failure due to CS being
> corrupted after a page fault.  The corruption was happening when the
> exception stack frame was not properly aligned and a page fault
> happened; the error info was then pushed after re-aligning the stack,
> so the value of eflags was actually written in CS place and other
> fields were shifted too.
> 
> It also makes sense to ensure this by adding two assertions, although
> these were primarly useful during debug.
> 
> Signed-off-by: Luca Dariz <luca@orpolo.org>

Applied, thanks!

> ---
>  i386/i386/pcb.c    | 10 +++++++++-
>  i386/i386/thread.h |  3 +++
>  i386/i386/trap.c   |  4 ++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/i386/i386/pcb.c b/i386/i386/pcb.c
> index 6e215b3e..4a6cbdb0 100644
> --- a/i386/i386/pcb.c
> +++ b/i386/i386/pcb.c
> @@ -147,6 +147,9 @@ void switch_ktss(pcb_t pcb)
>       pcb_stack_top = (pcb->iss.efl & EFL_VM)
>                       ? (long) (&pcb->iss + 1)
>                       : (long) (&pcb->iss.v86_segs);
> +#ifdef __x86_64__
> +     assert((pcb_stack_top & 0xF) == 0);
> +#endif
>  
>  #ifdef       MACH_RING1
>       /* No IO mask here */
> @@ -375,7 +378,12 @@ thread_t switch_context(
>  
>  void pcb_module_init(void)
>  {
> -     kmem_cache_init(&pcb_cache, "pcb", sizeof(struct pcb), 0,
> +     kmem_cache_init(&pcb_cache, "pcb", sizeof(struct pcb),
> +#ifdef __x86_64__
> +                     16,
> +#else
> +                     0,
> +#endif
>                       NULL, 0);
>  
>       fpu_module_init();
> diff --git a/i386/i386/thread.h b/i386/i386/thread.h
> index 4a9c1987..cb317bee 100644
> --- a/i386/i386/thread.h
> +++ b/i386/i386/thread.h
> @@ -200,6 +200,9 @@ struct i386_machine_state {
>  
>  typedef struct pcb {
>       struct i386_interrupt_state iis[2];     /* interrupt and NMI */
> +#ifdef __x86_64__
> +     unsigned long pad;         /* ensure exception stack is aligned to 16 */
> +#endif
>       struct i386_saved_state iss;
>       struct i386_machine_state ims;
>       decl_simple_lock_data(, lock)
> diff --git a/i386/i386/trap.c b/i386/i386/trap.c
> index 4f8612bc..23cb9f17 100644
> --- a/i386/i386/trap.c
> +++ b/i386/i386/trap.c
> @@ -361,6 +361,10 @@ int user_trap(struct i386_saved_state *regs)
>       int     type;
>       thread_t thread = current_thread();
>  
> +#ifdef __x86_64__
> +     assert(regs == &thread->pcb->iss);
> +#endif
> +
>       type = regs->trapno;
>       code = 0;
>       subcode = 0;
> -- 
> 2.30.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.