| Message ID | 20210429174650.8983-1-vincent.stehle@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: check query_variable_info attributes | expand |
On 4/29/21 7:46 PM, Vincent Stehlé wrote: > QueryVariableInfo() must return EFI_INVALID_PARAMETER when an invalid > combination of attribute bits is supplied. > > This fixes three SCT QueryVariableInfo_Conf failures. > > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com> > Reviewed-by: Grant Likely <grant.likely@arm.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Alexander Graf <agraf@csgraf.de> > > Changes since v1: > - Remove if/else and return directly > --- > lib/efi_loader/efi_var_common.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c > index b11ed91a74a..cbf8685fad5 100644 > --- a/lib/efi_loader/efi_var_common.c > +++ b/lib/efi_loader/efi_var_common.c > @@ -160,6 +160,10 @@ efi_status_t EFIAPI efi_query_variable_info( > EFI_ENTRY("%x %p %p %p", attributes, maximum_variable_storage_size, > remaining_variable_storage_size, maximum_variable_size); > > + if (!attributes || ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) && > + !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) > + return EFI_EXIT(EFI_INVALID_PARAMETER); Thank you for looking into closing this UEFI compliance gap. The checks must be executed at runtime too. efi_query_variable_info() only covers boot time. StandaloneMM has its own check in RuntimeServiceQueryVariableInfo(). That function only checks that attributes is non-zero. If you consider that check wrong, you would have to fix it there. Your U-Boot side checks should be put into efi_query_variable_info_int(). Why should we consider the following values valid? attributes == EFI_VARIABLE_APPEND_WRITE attributes == EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS attributes == EFI_VARIABLE_HARDWARE_ERROR_RECORD attributes == 0xffffff00 EFI_VARIABLE_APPEND_WRITE shall be ignored according to chapter 8.2 "Variable Services". Shouldn't we remove the bit before checking the remaining value? EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS for a variable that is neither BS nor RT makes little sense. According to chapter 8.2.4.2, "Hardware Error Record Variables" a hardware error variable must be NV, BS, RT, HR. 0xffffff00 has bits set that can never appear in attributes. efi_set_variable_int() would also accept some invalid combinations of flags. You could try to extract a common function for checking. Best regards Heinrich > + > ret = efi_query_variable_info_int(attributes, > maximum_variable_storage_size, > remaining_variable_storage_size, >
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index b11ed91a74a..cbf8685fad5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -160,6 +160,10 @@ efi_status_t EFIAPI efi_query_variable_info( EFI_ENTRY("%x %p %p %p", attributes, maximum_variable_storage_size, remaining_variable_storage_size, maximum_variable_size); + if (!attributes || ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) && + !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) + return EFI_EXIT(EFI_INVALID_PARAMETER); + ret = efi_query_variable_info_int(attributes, maximum_variable_storage_size, remaining_variable_storage_size,