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>
---
Changes since v1:
 - Copy back the results.
---
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 | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 375b94229e..f1cbf3088c 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,34 @@ 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);
+
+        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.33.0

On 18.11.2021 09:28, 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
> 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>

The code change looks correct to me, so it could have my R-b, but I'd
like to ask for some clarification first.

> --- 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,34 @@ 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.

I expect future gcc would also complain; I'm actually surprised it
doesn't already, as I recall work in that direction was done for one
of the more recent releases. Hence while I'm fine with referring to
clang specifically in the description, I'd prefer the comment here
to be more generic. E.g. "... and compilers may validly 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;

All three are OUT only as per the EFI spec. I'm not going to insist
on dropping these assignments, but their presence wants justifying
in the comment if you want to retain them. E.g. "While the function
parameters are OUT only, copy the values here anyway just in case."
Obviously if the assignments here were dropped, the local variables
would need to gain initializers to avoid leaking hypervisor stack
data.

If you agree with the suggested comment adjustments (and don't want
to e.g. go the route of dropping the assignments), I'd be happy to
make such adjustments while committing alongside adding my R-b. For
anything else I'd like to ask for a v3 submission.

Jan

On Thu, Nov 18, 2021 at 10:46:32AM +0100, Jan Beulich wrote:
> On 18.11.2021 09:28, 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
> > 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>
> 
> The code change looks correct to me, so it could have my R-b, but I'd
> like to ask for some clarification first.
> 
> > --- 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,34 @@ 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.
> 
> I expect future gcc would also complain; I'm actually surprised it
> doesn't already, as I recall work in that direction was done for one
> of the more recent releases. Hence while I'm fine with referring to
> clang specifically in the description, I'd prefer the comment here
> to be more generic. E.g. "... and compilers may validly complain."

Sure.

> > +         * 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;
> 
> All three are OUT only as per the EFI spec. I'm not going to insist
> on dropping these assignments, but their presence wants justifying
> in the comment if you want to retain them. E.g. "While the function
> parameters are OUT only, copy the values here anyway just in case."
> Obviously if the assignments here were dropped, the local variables
> would need to gain initializers to avoid leaking hypervisor stack
> data.

I think it's important to do the copy in order to prevent changes in
behavior. Even if explicitly listed as OUT I won't be surprised if
there where quirks that passed something in.

What about replacing the last paragraph with:

"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."

> If you agree with the suggested comment adjustments (and don't want
> to e.g. go the route of dropping the assignments), I'd be happy to
> make such adjustments while committing alongside adding my R-b. For
> anything else I'd like to ask for a v3 submission.

Sure.

Thanks, Roger.

On 18.11.2021 11:20, Roger Pau Monné wrote:
> On Thu, Nov 18, 2021 at 10:46:32AM +0100, Jan Beulich wrote:
>> On 18.11.2021 09:28, 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
>>> 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>
>>
>> The code change looks correct to me, so it could have my R-b, but I'd
>> like to ask for some clarification first.
>>
>>> --- 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,34 @@ 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.
>>
>> I expect future gcc would also complain; I'm actually surprised it
>> doesn't already, as I recall work in that direction was done for one
>> of the more recent releases. Hence while I'm fine with referring to
>> clang specifically in the description, I'd prefer the comment here
>> to be more generic. E.g. "... and compilers may validly complain."
> 
> Sure.
> 
>>> +         * 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;
>>
>> All three are OUT only as per the EFI spec. I'm not going to insist
>> on dropping these assignments, but their presence wants justifying
>> in the comment if you want to retain them. E.g. "While the function
>> parameters are OUT only, copy the values here anyway just in case."
>> Obviously if the assignments here were dropped, the local variables
>> would need to gain initializers to avoid leaking hypervisor stack
>> data.
> 
> I think it's important to do the copy in order to prevent changes in
> behavior. Even if explicitly listed as OUT I won't be surprised if
> there where quirks that passed something in.
> 
> What about replacing the last paragraph with:
> 
> "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."

Fine with me, thanks.

Jan

On 18/11/2021 10:20, Roger Pau Monné wrote:
> On Thu, Nov 18, 2021 at 10:46:32AM +0100, Jan Beulich wrote:
>> On 18.11.2021 09:28, 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
>>> 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>
>> The code change looks correct to me, so it could have my R-b, but I'd
>> like to ask for some clarification first.
>>
>>> --- 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,34 @@ 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.
>> I expect future gcc would also complain; I'm actually surprised it
>> doesn't already, as I recall work in that direction was done for one
>> of the more recent releases. Hence while I'm fine with referring to
>> clang specifically in the description, I'd prefer the comment here
>> to be more generic. E.g. "... and compilers may validly complain."
> Sure.
>
>>> +         * 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;
>> All three are OUT only as per the EFI spec. I'm not going to insist
>> on dropping these assignments, but their presence wants justifying
>> in the comment if you want to retain them. E.g. "While the function
>> parameters are OUT only, copy the values here anyway just in case."
>> Obviously if the assignments here were dropped, the local variables
>> would need to gain initializers to avoid leaking hypervisor stack
>> data.
> I think it's important to do the copy in order to prevent changes in
> behavior.

I agree with Jan here.  While I agree that not doing the copy in is
technically a change in behaviour, it's still the appropriate way to
express the logic.


> Even if explicitly listed as OUT I won't be surprised if
> there where quirks that passed something in.

I would be surprised, given how much static analysis typically goes into
the IN/OUT annotations in the windows world.

~Andrew