| Message ID | 3423244b0b1506d2a928799d80e15c19add75566.1702570086.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN,RFC] xen/bug: introduce bug_fn_nonconst_t to validate run_in_exception_handle() | expand |
On Thu, 14 Dec 2023, 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@bugseng.com> > --- > 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. > So, I came up with this solution, taking inspiration from the exising functions > show_execution_state() and show_execution_state_nonconst(). > > I would like to ask if: > 1) I correctly applied Julien's suggestion about the visibility [1]; > 2) this RFC can become a patch. > > [1] > https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01361.html > --- > xen/common/bug.c | 2 +- > xen/include/xen/bug.h | 21 +++++++++++++++------ > 2 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/xen/common/bug.c b/xen/common/bug.c > index ca166e102b..9170821ab8 100644 > --- a/xen/common/bug.c > +++ b/xen/common/bug.c > @@ -63,7 +63,7 @@ int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc) > > if ( id == BUGFRAME_run_fn ) > { > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug); > + bug_fn_nonconst_t *fn = bug_ptr(bug); > > fn(regs); > > 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 > @@ -12,6 +12,18 @@ > #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > > +#ifndef __ASSEMBLY__ > + > +/* > + * Make bug_fn_t and bug_fn_nonconst_t visible for arch-specific implementation > + * of run_in_exception_handler. > + */ > +struct cpu_user_regs; > +typedef void bug_fn_t(const struct cpu_user_regs *regs); > +typedef void bug_fn_nonconst_t(struct cpu_user_regs *regs); > + > +#endif > + > #include <asm/bug.h> > > #ifndef __ASSEMBLY__ > @@ -99,18 +111,15 @@ struct bug_frame { > > #endif > > -struct cpu_user_regs; > -typedef void bug_fn_t(const struct cpu_user_regs *regs); I am not sure why you moved this up in the file, but everything looks correct to me. I would ack it but I'll let Julien confirm in regards to his older comment. > #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(). > */ > -#define run_in_exception_handler(fn) do { \ > - (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \ > - BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ > +#define run_in_exception_handler(fn) do { \ > + (void)((fn) == (bug_fn_nonconst_t *)NULL); \ > + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ > } while ( false ) > > #endif /* run_in_exception_handler */ > -- > 2.34.1 >
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@bugseng.com> > --- > 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
diff --git a/xen/common/bug.c b/xen/common/bug.c index ca166e102b..9170821ab8 100644 --- a/xen/common/bug.c +++ b/xen/common/bug.c @@ -63,7 +63,7 @@ int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc) if ( id == BUGFRAME_run_fn ) { - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug); + bug_fn_nonconst_t *fn = bug_ptr(bug); fn(regs); 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 @@ -12,6 +12,18 @@ #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) +#ifndef __ASSEMBLY__ + +/* + * Make bug_fn_t and bug_fn_nonconst_t visible for arch-specific implementation + * of run_in_exception_handler. + */ +struct cpu_user_regs; +typedef void bug_fn_t(const struct cpu_user_regs *regs); +typedef void bug_fn_nonconst_t(struct cpu_user_regs *regs); + +#endif + #include <asm/bug.h> #ifndef __ASSEMBLY__ @@ -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(). */ -#define run_in_exception_handler(fn) do { \ - (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \ - BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ +#define run_in_exception_handler(fn) do { \ + (void)((fn) == (bug_fn_nonconst_t *)NULL); \ + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ } while ( false ) #endif /* run_in_exception_handler */
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@bugseng.com> --- 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. So, I came up with this solution, taking inspiration from the exising functions show_execution_state() and show_execution_state_nonconst(). I would like to ask if: 1) I correctly applied Julien's suggestion about the visibility [1]; 2) this RFC can become a patch. [1] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01361.html --- xen/common/bug.c | 2 +- xen/include/xen/bug.h | 21 +++++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-)