Re: [PATCH 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> When compiling with any ARM toolchain and Os, registers can get
> trashed when returning for the second time from SetJump because GCC
> only handles this correctly when using standard names like 'setjmp' or
> 'getcontext'. When different names are used you have to use the
> attribute 'returns_twice' to tell gcc to be extra careful.
>
> example:
> extern int  FN_NAME(void*);
>
> void jmp_buf_set(void *jmpb, void (*f)(void))
> {
>   if (!FN_NAME(jmpb))
>     f();
> }
>
> this code produces this wrong code with Os:
> 00000000 <jmp_buf_set>:
>    0: e92d4010 push {r4, lr}
>    4: e1a04001 mov r4, r1
>    8: ebfffffe bl 0 <nonstandard_setjmp>
>    c: e3500000 cmp r0, #0
>   10: 01a03004 moveq r3, r4
>   14: 08bd4010 popeq {r4, lr}
>   18: 012fff13 bxeq r3
>   1c: e8bd4010 pop {r4, lr}
>   20: e12fff1e bx lr
>
> The generated code pushes backups of r4 and lr to the stack and then
> saves all registers using nonstandard_setjmp.
> Then it pops the stack and jumps to the function in r3 which is the
> main problem because now the function can overwrite our register
> backups on the stack.
> When we return a second time from the call to nonstandard_setjmp, the
> stack pointer has it's original(pushed) position and when the code
> pops r4 and lr from the stack the values are not guaranteed to be the
> same.
>
> When using a standard name like setjmp or getcontext or adding
> '__attribute__((returns_twice))' to nonstandard_setjmp's declaration
> the code looks different:
>
> 00000000 <jmp_buf_set>:
>    0: e92d4007 push {r0, r1, r2, lr}
>    4: e58d1004 str r1, [sp, #4]
>    8: ebfffffe bl 0 <setjmp>
>    c: e3500000 cmp r0, #0
>   10: 059d3004 ldreq r3, [sp, #4]
>   14: 01a0e00f moveq lr, pc
>   18: 012fff13 bxeq r3
>   1c: e28dd00c add sp, sp, #12
>   20: e49de004 pop {lr} ; (ldr lr, [sp], #4)
>   24: e12fff1e bx lr
>
> Here the problem is being solved by restoring r3 from the stack
> without popping it.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

Thanks for fixing this. Note that this affects all architectures
targeted by Clang/GCC, not just 32-bit ARM.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  MdePkg/Include/Library/BaseLib.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index 2b98af4cd17e..10976032adaa 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4905,6 +4905,7 @@ MemoryFence (
>  **/
>  UINTN
>  EFIAPI
> +RETURNS_TWICE
>  SetJump (
>    OUT     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
>    );
> --
> 2.15.1
>