Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH for-4.16] efi: fix alignment of function 
parameters in compat mode"):
> Currently the max_store_size, remain_store_size and max_size in
> compat_pf_efi_runtime_call are 4 byte aligned, which makes clang
> complain with:
> 
> In file included from compat.c:30:
> ./runtime.c:646:13: error: passing 4-byte aligned argument to 8-byte aligned 
> parameter 2 of 'QueryVariableInfo' may result in an unaligned pointer access 
> [-Werror,-Walign-mismatch]
>             &op->u.query_variable_info.max_store_size,
>             ^
> ./runtime.c:647:13: error: passing 4-byte aligned argument to 8-byte aligned 
> parameter 3 of 'QueryVariableInfo' may result in an unaligned pointer access 
> [-Werror,-Walign-mismatch]
>             &op->u.query_variable_info.remain_store_size,
>             ^
> ./runtime.c:648:13: error: passing 4-byte aligned argument to 8-byte aligned 
> parameter 4 of 'QueryVariableInfo' may result in an unaligned pointer access 
> [-Werror,-Walign-mismatch]
>             &op->u.query_variable_info.max_size);
>             ^
> Fix this by bouncing the variables on the stack in order for them to
> be 8 byte aligned.
> 
> Note this could be done in a more selective manner to only apply to
> compat code calls, but given the overhead of making an EFI call doing
> an extra copy of 3 variables doesn't seem to warrant the special
> casing.
...
> Tagged for possible inclusion into the release, as it's a likely
> candidate for backport. It shouldn't introduce any functional change
> from a caller PoV. I think the risk is getting the patch wrong and not
> passing the right parameters, or broken EFI implementations corrupting
> data on our stack instead of doing it in xenpf_efi_runtime_call.

Thanks.  I have double-checked the variable names etc.

Reviewed-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

I agree with your analysis vis-a-vis 4.16.  The current code is
technically UB[1] and it seems possible that it might trigger bugs in
firmware.

I would like a third opinion (even though technically my review might
be enough).  So, subject to a review from a hypervisor maintainer:

Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

Thanks,
Ian.

[1] Well, as far as I can tell.  My copy of C99+TC1+TC2 is hopelessly
unclear about unaligned pointers, and here of course we have a
compiler extension too.