From: Roger Pau Monne <roger.pau@citrix.com>

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@citrix.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
Signed-off-by: Ian Jackson <iwj@xenproject.org>

---
Changes since v2:
 - Adjust the commentary as per discussion.
Changes since v1:
 - Copy back the results.
---
 xen/common/efi/runtime.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 375b94229e..089bb0eb1b 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,36 @@ 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
+         * and compilers may validly complain.
+	 *
+	 * Note that while the function parameters are OUT only, copy the
+	 * values here anyway just in case. This is done 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);
+
+        op->u.query_variable_info.max_store_size = max_store_size;
+        op->u.query_variable_info.remain_store_size = remain_store_size;
+        op->u.query_variable_info.max_size = max_size;
+
         break;
+    }
 
     case XEN_EFI_query_capsule_capabilities:
     case XEN_EFI_update_capsule:
-- 
2.20.1

On 19/11/2021 15:24, Ian Jackson wrote:
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 375b94229e..089bb0eb1b 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -638,16 +641,36 @@ 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
> +         * and compilers may validly complain.
> +	 *
> +	 * Note that while the function parameters are OUT only, copy the
> +	 * values here anyway just in case. This is done regardless of whether
> +	 * called from the compat handler or not, as it's not worth the extra
> +	 * logic to differentiate.
> +         */

Some hardtabs appear to have slipped in.

Jan gave a conditional R-by which permitted a change along these lines,
but he's left the office now too, so you'll have to take him up on that
offer if you want this committing before Monday.

~Andrew

Andrew Cooper writes ("Re: [PATCH for-4.16 v3] efi: fix alignment of function parameters in compat mode"):
> Some hardtabs appear to have slipped in.

Thanks.  Fixed.

> Jan gave a conditional R-by which permitted a change along these lines,
> but he's left the office now too, so you'll have to take him up on that
> offer if you want this committing before Monday.

Right.

FTAOD I think I have done what Jan approved, and Andy double checked
that.  So I am about to push this.  For the record, final version
below.

Ian.

From be12fcca8b784e456df3adedbffe657d753c5ff9 Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@citrix.com>
Date: Thu, 18 Nov 2021 09:28:06 +0100
Subject: [PATCH for-4.16 v3] efi: fix alignment of function parameters in
 compat mode
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Cc: Andrew Cooper <amc96@srcf.net>

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@citrix.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
Signed-off-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---
Changes since v3:
 - Remove hard tabs.  Apply Jan's r-b as authorised in email.
Changes since v2:
 - Adjust the commentary as per discussion.
Changes since v1:
 - Copy back the results.
---
 xen/common/efi/runtime.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 375b94229e..d2fdc28df3 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,36 @@ 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
+         * and compilers may validly complain.
+         *
+         * Note that while the function parameters are OUT only, copy the
+         * values here anyway just in case. This is done 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);
+
+        op->u.query_variable_info.max_store_size = max_store_size;
+        op->u.query_variable_info.remain_store_size = remain_store_size;
+        op->u.query_variable_info.max_size = max_size;
+
         break;
+    }
 
     case XEN_EFI_query_capsule_capabilities:
     case XEN_EFI_update_capsule:
-- 
2.20.1

On 19.11.2021 18:09, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH for-4.16 v3] efi: fix alignment of function parameters in compat mode"):
>> Some hardtabs appear to have slipped in.
> 
> Thanks.  Fixed.
> 
>> Jan gave a conditional R-by which permitted a change along these lines,
>> but he's left the office now too, so you'll have to take him up on that
>> offer if you want this committing before Monday.
> 
> Right.
> 
> FTAOD I think I have done what Jan approved, and Andy double checked
> that.  So I am about to push this.  For the record, final version
> below.

Right, thanks. Nevertheless I think for 4.17 we will want to ...

> --- 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,36 @@ 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
> +         * and compilers may validly complain.
> +         *
> +         * Note that while the function parameters are OUT only, copy the
> +         * values here anyway just in case. This is done 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;

... eliminate these.

Jan