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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2021 09:28, Roger Pau Monne wrote:
> Currently the max_store_size, remain_store_size and max_size in
> compat_pf_efi_runtime_call are 4 byte aligned, which makes clang
> 13.0.0 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.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

The code change looks correct to me, so it could have my R-b, but I'd
like to ask for some clarification first.

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -607,6 +607,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>      break;
>  
>      case XEN_EFI_query_variable_info:
> +    {
> +        uint64_t max_store_size, remain_store_size, max_size;
> +
>          if ( op->misc & ~XEN_EFI_VARINFO_BOOT_SNAPSHOT )
>              return -EINVAL;
>  
> @@ -638,16 +641,34 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>  
>          if ( !efi_enabled(EFI_RS) || (efi_rs->Hdr.Revision >> 16) < 2 )
>              return -EOPNOTSUPP;
> +
> +        /*
> +         * Bounce the variables onto the stack to make them 8 byte aligned 
> when
> +         * called from the compat handler, as their placement in
> +         * compat_pf_efi_runtime_call will make them 4 byte aligned instead 
> and
> +         * clang will complain.

I expect future gcc would also complain; I'm actually surprised it
doesn't already, as I recall work in that direction was done for one
of the more recent releases. Hence while I'm fine with referring to
clang specifically in the description, I'd prefer the comment here
to be more generic. E.g. "... and compilers may validly complain."

> +         * Note we do this regardless of whether called from the compat 
> handler
> +         * or not, as it's not worth the extra logic to differentiate.
> +         */
> +        max_store_size = op->u.query_variable_info.max_store_size;
> +        remain_store_size = op->u.query_variable_info.remain_store_size;
> +        max_size = op->u.query_variable_info.max_size;

All three are OUT only as per the EFI spec. I'm not going to insist
on dropping these assignments, but their presence wants justifying
in the comment if you want to retain them. E.g. "While the function
parameters are OUT only, copy the values here anyway just in case."
Obviously if the assignments here were dropped, the local variables
would need to gain initializers to avoid leaking hypervisor stack
data.

If you agree with the suggested comment adjustments (and don't want
to e.g. go the route of dropping the assignments), I'd be happy to
make such adjustments while committing alongside adding my R-b. For
anything else I'd like to ask for a v3 submission.

Jan