Re: [kvm-unit-tests v3 03/13] powerpc: Add some checking to exception handler install

Nicholas Piggin
2023-03-28T06:53:34Z

On Tue Mar 28, 2023 at 12:39 AM AEST, Thomas Huth wrote:
> On 27/03/2023 14.45, Nicholas Piggin wrote:
> > Check to ensure exception handlers are not being overwritten or
> > invalid exception numbers are used.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Since v2:
> > - New patch
> >
> > lib/powerpc/processor.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index ec85b9d..70391aa 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -19,11 +19,23 @@ static struct {
> > void handle_exception(int trap, void (*func)(struct pt_regs *, void *),
> > void * data)
> > {
> > + if (trap & 0xff) {
>
> You could check for the other "invalid exception handler" condition here
> already, i.e. if (trap & ~0xf00) ...
>
> I'd maybe simply do an "assert(!(trap & ~0xf00))" here.
>
> > + printf("invalid exception handler %#x\n", trap);
> > + abort();
> > + }
> > +
> > trap >>= 8;
> >
> > if (trap < 16) {
>
> ... then you could get rid of the if-statement here and remove one level of
> indentation in the code below.

Yes that's the way to do it. I feel embarrassed for not thinking
of it :)

Thanks,
Nick

>
> > + if (func && handlers[trap].func) {
> > + printf("exception handler installed twice %#x\n", trap);
> > + abort();
> > + }
> > handlers[trap].func = func;
> > handlers[trap].data = data;
> > + } else {
> > + printf("invalid exception handler %#x\n", trap);
> > + abort();
> > }
> > }
> >
>
> Thomas