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

[Andrew Cooper <amc96@xxxxxxxx>]

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@xxxxxxxxxx>
>>> Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
>> 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