[U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT

[Rob Clark]

On Wed, Jul 26, 2017 at 4:12 PM, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 26.07.17 18:41, Rob Clark wrote:
>>
>> On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>>
>>> On 26.07.17 15:55, Rob Clark wrote:
>>>>
>>>>
>>>> Missing an EFI_ENTRY() or doubling up EFI_EXIT() leads to non-obvious
>>>> crashes.  Let's add some error checking.
>>>>
>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>> ---
>>>>    include/efi_loader.h          |  5 +++++
>>>>    lib/efi_loader/efi_boottime.c | 13 +++++++++++++
>>>>    2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 09bab7dbc6..4b49fac84b 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -15,11 +15,13 @@
>>>>      #include <linux/list.h>
>>>>    +int __efi_check_nesting(int delta);
>>>>    /*
>>>>     * Enter the u-boot world from UEFI:
>>>>     */
>>>>    #define EFI_ENTRY(format, ...) do { \
>>>>          efi_restore_gd(); \
>>>> +       assert(__efi_check_nesting(+1)); \
>>>>          debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__);
>>>> \
>>>>          } while(0)
>>>>    @@ -28,6 +30,7 @@
>>>>     */
>>>>    #define EFI_EXIT(ret) ({ \
>>>>          debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &
>>>> ~EFI_ERROR_MASK)); \
>>>> +       assert(__efi_check_nesting(-1)); \
>>>>          efi_exit_func(ret); \
>>>>          })
>>>>    @@ -36,10 +39,12 @@
>>>>     */
>>>>    #define EFI_CALL(exp) do { \
>>>>          debug("EFI: Call: %s\n", #exp); \
>>>> +       assert(__efi_check_nesting(-1)); \
>>>>          efi_exit_func(EFI_SUCCESS); \
>>>>          exp; \
>>>>          efi_restore_gd(); \
>>>>          debug("EFI: Return From: %s\n", #exp); \
>>>> +       assert(__efi_check_nesting(+1)); \
>>>>          } while(0)
>>>>      extern struct efi_runtime_services efi_runtime_services;
>>>> diff --git a/lib/efi_loader/efi_boottime.c
>>>> b/lib/efi_loader/efi_boottime.c
>>>> index 76cafffc1d..b21df7bd5d 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -57,6 +57,17 @@ void efi_save_gd(void)
>>>>    #endif
>>>>    }
>>>>    +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
>>>> +int __efi_check_nesting(int delta)
>>>> +{
>>>> +       static int entry_count;
>>>
>>>
>>>
>>> I'd prefer if we marked globals as such (read: somewhere at the top of a
>>> .c
>>> file).
>>
>>
>> hmm, well that does increase the scope unnecessarily, which is why I
>> made it static inside the fxn.  But if you can prefer I guess I can
>> put it just above __efi_check_nesting().
>
>
> It's a matter of taste, but in general I find it very useful to know at a
> glimpse where .bss, .rodata and .data come from.
>
>>
>>> Also I think this function would be better off as a static inline in a
>>> header, as it should get compressed quite well.
>>
>>
>> That would mean making entry_count also non-static (ie. broader than
>
>
> Ah, no, entry_count would still be in efi_boottime.c, but the function using
> it would be in a .h file. That way we don't need to do the register
> save/restore dance we need to do for full remote function calls.

Ok.. I'm thinking about just moving these calls into
efi_restore_gd()/efi_exit_func() but leaving the asserts in the macro
(so the assert reflects the proper file/line).  Minor downside is the
assert would come before the debug() print, but since it has file/line
I guess that isn't really important.

Fortunately the one place we need to special-case call the
check_nesting() (ie. efi_start_image()) is in the same object file.

I'll give that a go this evening and see how it turns out.

BR,
-R