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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <iwj@xenproject.org>

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.
---
 xen/common/efi/runtime.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 375b94229e..4462233798 100644
--- 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,29 @@ 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.
+         *
+         * 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;
+
         state = efi_rs_enter();
         if ( !state.cr3 )
             return -EOPNOTSUPP;
         status = efi_rs->QueryVariableInfo(
-            op->u.query_variable_info.attr,
-            &op->u.query_variable_info.max_store_size,
-            &op->u.query_variable_info.remain_store_size,
-            &op->u.query_variable_info.max_size);
+            op->u.query_variable_info.attr, &max_store_size, &remain_store_size,
+            &max_size);
         efi_rs_leave(&state);
         break;
+    }
 
     case XEN_EFI_query_capsule_capabilities:
     case XEN_EFI_update_capsule:
-- 
2.33.0

On 17/11/2021 15:33, 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
> 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@citrix.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
>
> 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.
> ---
>   xen/common/efi/runtime.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 375b94229e..4462233798 100644
> --- 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,29 @@ 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.
> +         *
> +         * 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;
> +
>           state = efi_rs_enter();
>           if ( !state.cr3 )
>               return -EOPNOTSUPP;
>           status = efi_rs->QueryVariableInfo(
> -            op->u.query_variable_info.attr,
> -            &op->u.query_variable_info.max_store_size,
> -            &op->u.query_variable_info.remain_store_size,
> -            &op->u.query_variable_info.max_size);
> +            op->u.query_variable_info.attr, &max_store_size, &remain_store_size,
> +            &max_size);
>           efi_rs_leave(&state);

This will compile, but the caller won't get any data back unless you 
copy the opposite way here...

~Andrew

>           break;
> +    }
>   
>       case XEN_EFI_query_capsule_capabilities:
>       case XEN_EFI_update_capsule:

Andrew Cooper writes ("Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode"):
> This will compile, but the caller won't get any data back unless you 
> copy the opposite way here...

Well spotted.  I feel quite the fool!

Ian.

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@xenproject.org>

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@xenproject.org>

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.