Re: [XEN RFC] xen/bug: introduce bug_fn_nonconst_t to validate run_in_exception_handle()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/2023 4:35 pm, Federico Serafini wrote:
> Introduce function type bug_fn_nonconst_t (as opposed to bug_fn_t)
> to validate the argument passed to run_in_exception_handle().
>
> Place the definition of bug_fn_nonconst_t before of asm/bug.h inclusion
> so that arch-specific implementations of run_in_exception_handler() can
> use it (and move bug_fn_t into the same place for the same reason).
>
> Furthermore, use bug_fn_nonconst_t to address violations of
> MISRA C:2012 Rule 8.2  ("Function types shall be in prototype form with
> named parameters").
>
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
> At the moment, bug_fn_t can not be used within run_in_exception_handler()
> because of the const qualifier on the formal parameter.
> I tried to adjust the constness of the functions passed to
> run_in_exception_handler but at a certain point I stopped:
> a lot of code churn is required and I am not sure about the correctenss of the
> result.

run_in_exception_handler() should be bug_fn_t.  Any mutation of the
pointer passed in is UB, as it corrupts state behind C's back.

In fact, it's not right for any of the IRQ infrastructure to take a regs
pointer, because the pointer can't be safely be mutated reasons, *and*
its not relevant content for an interrupt handler to peek at (except
perhaps a profiler).  Furthermore, there is even a way to recover the
regs pointer if you really need it, via get_irq_regs().

>From a safety perspective, dropping the regs parameter would be a very
good thing.  It shrinks the compiled binary (don't need to spill and
recover the pointer in all the infrastructure), and it will avoid
needing to answer awkward questions such as "why are you passing around
a pointer that you can't legally read or write?"  But it's also a lot of
work and definitely out of scope for run_in_exception_handler()

> So, I came up with this solution, taking inspiration from the exising 
> functions
> show_execution_state() and show_execution_state_nonconst().

show_execution_state_nonconst() is a bodge of mine to use a const-taking
function in an API wanting a non-const pointer, and it only exists
because I didn't have time to untangle this mess while working to
security embargo.

The only path I can see which actually mutates the pointer is
do_debug_key() into debugger_trap_fatal() into GDBstub's
process_command().  However, it's a debugger making arbitrary state
changes, so can cast away const in order to do so.

> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> index cb5138410e..c6f5594af5 100644
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -99,18 +111,15 @@ struct bug_frame {
>  
>  #endif
>  
> -struct cpu_user_regs;
> -typedef void bug_fn_t(const struct cpu_user_regs *regs);
> -
>  #ifndef run_in_exception_handler
>  
>  /*
>   * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
>   * and use a real static inline here to get proper type checking of fn().
>   */

I'm pretty sure this TODO can be completed now that we have xen/macros.h

In fact, the fact there's a common run_in_exception_handler() provided
now also helps simplify some of the other cases.


I would much rather that we fix run_in_exception_handler() to use
bug_fn_t than to continue hacking around the breakage.  I really don't
think it's terribly complicated to fix now.

~Andrew