Re: x86/irq: Make run_on_irqstack_cond() typesafe

[Jann Horn]

On Wed, Sep 23, 2020 at 9:20 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Tue, Sep 22, 2020 at 09:58:52AM +0200, Thomas Gleixner wrote:
> > -void asm_call_on_stack(void *sp, void *func, void *arg);
> > +void asm_call_on_stack(void *sp, void (*func)(void), void *arg);
> > +void asm_call_sysvec_on_stack(void *sp, void (*func)(struct pt_regs *regs),
> > +                           struct pt_regs *regs);
> > +void asm_call_irq_on_stack(void *sp, void (*func)(struct irq_desc *desc),
> > +                        struct irq_desc *desc);
>
> Eeeh, err. So, this is nice for the CFI case, but can we instead just
> inline asm_call_on_stack() instead? Having any of these as distinct
> functions in the kernel is really not safe: it provides a trivial
> global stack-pivot[1] function for use in ROP attacks, which is one
> of the central requirements for mounting such attacks. This allows a
> completely arbitrary sp argument, function, and first argument. :(
>
> Much better would be to keep asm_call_on_stack() as an inline so the
> stack pointer is always coming from percpu variables, and to have the
> irq_count actually checked (i.e. freak out if it falls below zero to
> catch jumps into the middle of a function when an attempt to bypass the
> load from the percpu area happens). I would expect this form to be much
> robust:
>
> inc
> load sp from per-cpu
> pivot sp
> make call
> restore sp
> WARN(dec_and_test)

I don't see the point. If you can already jump to arbitrary kernel
instructions, I would be extremely surprised if you could't find some
other way to get full kernel read/write. Even just jumping to the
epilogue of some function that increments the stack pointer and then
tries to return (maybe even after loading RBP from that spot on the
stack) will probably get you quite far.