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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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