[PATCH] EFI: Fix getting EFI variable list on some systems

[PATCH] EFI: Fix getting EFI variable list on some systems

[Ross Lagerwall]

Copy the entire output buffer to the guest because it may contain data beyond
the output size that the firmware requires on a subsequent
GetNextVariableName() call (e.g. a NULL character).

The spec requires that on each call, "the previous results" be passed in.

Without this change, the following (simplified) sequence would occur:
GetNextVariableName: in \0, out AdminPw\0, size 7
GetNextVariableName: in AdminPw\0, out UserPw\0, size 6
GetNextVariableName: in UserPww\0, NOT FOUND

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/efi/runtime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 7ed5bfa..20772f3 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -518,7 +518,7 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 
             if ( !EFI_ERROR(status) &&
                  copy_to_guest(op->u.get_next_variable_name.name,
-                               name.raw, size) )
+                               name.raw, op->u.get_next_variable_name.size) )
                 rc = -EFAULT;
             op->u.get_next_variable_name.size = size;
         }
-- 
2.1.0

Re: [PATCH] EFI: Fix getting EFI variable list on some systems

[Andrew Cooper]

On 09/03/15 16:47, Ross Lagerwall wrote:
> Copy the entire output buffer to the guest because it may contain data beyond
> the output size that the firmware requires on a subsequent
> GetNextVariableName() call (e.g. a NULL character).
>
> The spec requires that on each call, "the previous results" be passed in.
>
> Without this change, the following (simplified) sequence would occur:
> GetNextVariableName: in \0, out AdminPw\0, size 7
> GetNextVariableName: in AdminPw\0, out UserPw\0, size 6
> GetNextVariableName: in UserPww\0, NOT FOUND
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/common/efi/runtime.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 7ed5bfa..20772f3 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -518,7 +518,7 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>  
>              if ( !EFI_ERROR(status) &&
>                   copy_to_guest(op->u.get_next_variable_name.name,
> -                               name.raw, size) )
> +                               name.raw, op->u.get_next_variable_name.size) )
>                  rc = -EFAULT;
>              op->u.get_next_variable_name.size = size;
>          }

Re: [PATCH] EFI: Fix getting EFI variable list on some systems

[Jan Beulich]

>>> On 09.03.15 at 17:47, <ross.lagerwall@citrix.com> wrote:
> Copy the entire output buffer to the guest because it may contain data beyond
> the output size that the firmware requires on a subsequent
> GetNextVariableName() call (e.g. a NULL character).
> 
> The spec requires that on each call, "the previous results" be passed in.
> 
> Without this change, the following (simplified) sequence would occur:
> GetNextVariableName: in \0, out AdminPw\0, size 7
> GetNextVariableName: in AdminPw\0, out UserPw\0, size 6
> GetNextVariableName: in UserPww\0, NOT FOUND

As such behavior is outside the specification, please name the system
needing this workaround: The runtime services function is documented
to update *VariableNameSize only upon EFI_BUFFER_TOO_SMALL. A
code comment would also seem to be on order, as otherwise people
like me might be tempted to undo this again, as it's sub-optimal code
for spec conforming firmware.

And to save me from having to do an incremental patch on top, you
may want to consider switching to __copy_to_user() at once.

Jan