[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxx>
---
Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>

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:




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.