* [PATCH for-4.16] efi: fix alignment of function parameters in compat mode
@ 2021-11-17 15:33 Roger Pau Monne
2021-11-17 15:43 ` Andrew Cooper
2021-11-17 16:04 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Ian Jackson
0 siblings, 2 replies; 6+ messages in thread
From: Roger Pau Monne @ 2021-11-17 15:33 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Ian Jackson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode
2021-11-17 15:33 [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Roger Pau Monne
@ 2021-11-17 15:43 ` Andrew Cooper
2021-11-17 16:05 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages] Ian Jackson
2021-11-17 16:04 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Ian Jackson
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2021-11-17 15:43 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich, Ian Jackson
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:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode
2021-11-17 15:33 [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Roger Pau Monne
2021-11-17 15:43 ` Andrew Cooper
@ 2021-11-17 16:04 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2021-11-17 16:04 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Jan Beulich
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages]
2021-11-17 15:43 ` Andrew Cooper
@ 2021-11-17 16:05 ` Ian Jackson
2021-11-17 16:57 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Ian Jackson @ 2021-11-17 16:05 UTC (permalink / raw)
To: Andrew Cooper, Roger Pau Monne; +Cc: xen-devel, Jan Beulich
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages]
2021-11-17 16:05 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages] Ian Jackson
@ 2021-11-17 16:57 ` Roger Pau Monné
2021-11-17 18:36 ` Ian Jackson
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2021-11-17 16:57 UTC (permalink / raw)
To: Ian Jackson; +Cc: Andrew Cooper, xen-devel, Jan Beulich
On Wed, Nov 17, 2021 at 04:05:43PM +0000, Ian Jackson wrote:
> 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!
Indeed. Will send a fixed version tomorrow. Would you like me to keep
the release ack Ian?
Thanks, Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages]
2021-11-17 16:57 ` Roger Pau Monné
@ 2021-11-17 18:36 ` Ian Jackson
0 siblings, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2021-11-17 18:36 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, xen-devel, Jan Beulich
Roger Pau Monné writes ("Re: [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages]"):
> On Wed, Nov 17, 2021 at 04:05:43PM +0000, Ian Jackson wrote:
> > 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!
>
> Indeed. Will send a fixed version tomorrow. Would you like me to keep
> the release ack Ian?
Yes, please.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-17 18:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 15:33 [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Roger Pau Monne
2021-11-17 15:43 ` Andrew Cooper
2021-11-17 16:05 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode [and 1 more messages] Ian Jackson
2021-11-17 16:57 ` Roger Pau Monné
2021-11-17 18:36 ` Ian Jackson
2021-11-17 16:04 ` [PATCH for-4.16] efi: fix alignment of function parameters in compat mode Ian Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).