| Message ID | 20211118082806.23335-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.16,v2] efi: fix alignment of function parameters in compat mode | expand |
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
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:
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(-)