Re: [PATCH v2 4/5] lib: sbi: Create a spot to place Smrnmi detection before traps and after DT is ready

Hi Anup,

Two concerns about the suggested approach.

1) Enablement of NMIs before the platform has set up its NMI handler
introduces a window of possible hang. While NMIE=1, the trap handler is
not defined, so if an NMI arrives, the trap jumps to nowhere.

The v2 series keeps NMIE=0 until smrnmi_handlers_init() has installed
both NMIVEC and the regular trap entry, which avoids this hole.

More generally: on Smrnmi-enabled platforms, there is really no way to
use any trap-based mechanism until the platform has set its NMI handlers.
Holding off them until after a call to the platform's
smrnmi_handlers_init() is mandatory to prevent possible hangs.

2) About the stack canary concern with the C-side __stack_chk_guard_init().

The danger pattern "canary already pushed on stack" only triggers if a
function call which returns is currently on stack, which means
__stack_chk_guard variable will be checked against its value on prologue.
Neither C nor asm functions currently on stack return or use epilogue
check of __stack_chk_guard. The full call chain to __stack_chk_guard_init()
consists of:

  asm _start
   -> sbi_init()              __noreturn
      -> init_coldboot()      __noreturn
         -> __stack_chk_guard_init()    asm, no canary

Even if asm side falls back to `return`, it is still `j _start_hang`
which also doesn't check epilogue.

Could you reconsider patch 4/5 with this context?

Thanks,
Evgeny

-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi