| Message ID | 20211119152403.12069-1-iwj@xenproject.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.16,v3] efi: fix alignment of function parameters in compat mode | expand |
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:
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
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: