Re: [PATCH 15/16] efi_loader: enable UEFI variables at runtime

2020-04-01 Thread AKASHI Takahiro 
On Wed, Apr 01, 2020 at 08:26:24AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:41 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
> >> Enable UEFI variables at runtime.
> >>
> >> Signed-off-by: Heinrich Schuchardt 
> >> ---
> >>  lib/efi_loader/efi_runtime.c  |  6 +-
> >>  lib/efi_loader/efi_variable.c | 23 +--
> >>  2 files changed, 22 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >> index 664a0422e2..acd644202d 100644
> >> --- a/lib/efi_loader/efi_runtime.c
> >> +++ b/lib/efi_loader/efi_runtime.c
> >> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
> >>rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
> >>rt_table->length = sizeof(struct efi_rt_properties_table);
> >>rt_table->runtime_services_supported =
> >> +  EFI_RT_SUPPORTED_GET_VARIABLE |
> >> +  EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
> >> +  EFI_RT_SUPPORTED_SET_VARIABLE |
> >>EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> >> -  EFI_RT_SUPPORTED_CONVERT_POINTER;
> >> +  EFI_RT_SUPPORTED_CONVERT_POINTER |
> >> +  EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> >>
> >>/*
> >> * This value must be synced with efi_runtime_detach_list
> >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >> index 7c39542968..cf8b44c535 100644
> >> --- a/lib/efi_loader/efi_variable.c
> >> +++ b/lib/efi_loader/efi_variable.c
> >> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI 
> >> efi_query_variable_info(
> >>u64 *remaining_variable_storage_size,
> >>u64 *maximum_variable_size)
> >>  {
> >> -  /*
> >>*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
> >> sizeof(struct efi_var_file);
> >>*remaining_variable_storage_size = efi_var_mem_free();
> >>*maximum_variable_size = EFI_VAR_BUF_SIZE -
> >> sizeof(struct efi_var_file) -
> >> sizeof(struct efi_var_entry);
> >> -  */
> >> -  return EFI_UNSUPPORTED;
> >> +  return EFI_SUCCESS;
> >>  }
> >>
> >>  /**
> >> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
> >>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
> >> u32 *attributes, efi_uintn_t *data_size, void *data)
> >>  {
> >> -  return EFI_UNSUPPORTED;
> >> +  return efi_get_variable_int(variable_name, vendor, attributes,
> >> +  data_size, data);
> >>  }
> >>
> >>  /**
> >> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
> >>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>   u16 *variable_name, efi_guid_t *vendor)
> >>  {
> >> -  return EFI_UNSUPPORTED;
> >> +  return efi_get_next_variable_name_int(variable_name_size, variable_name,
> >> +vendor);
> >>  }
> >>
> >>  /**
> >> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const 
> >> efi_guid_t *vendor,
> >> u32 attributes, efi_uintn_t data_size,
> >> const void *data)
> >>  {
> >> -  return EFI_UNSUPPORTED;
> >> +  const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
> >> +  EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +  EFI_VARIABLE_RUNTIME_ACCESS;
> >> +
> >
> > Why not support APPEND_WRITE?
> 
> This is the list of attributes that have to be set as a *minimum* in a
> valid call to SetVariable. APPEND_WRITE is not required but optional.
> 
> >
> >
> >> +  if (attributes &&
> >> +  (attributes & required_attributes) != required_attributes)
> >> +  return EFI_INVALID_PARAMETER;
> >
> > UEFI specification says,
> > ===8<
> > Variables that have runtime access but that are not nonvolatile are read-
> > only data variables once ExitBootServices() is performed.
> > [snip]
> > EFI_WRITE_PROTECTED The variable in question is read-only.
> > ===>8===
> >
> > So in this case, we should return WRITE_PROTECTED instead of
> > INVALID_PARAMETER.
> 
> Here I check the parameters of the call which may create a new variable.

If !NON_VOLATILE, 
> >> +  if (attributes &&
> >> +  (attributes & required_attributes) != required_attributes)
> >> +  return EFI_INVALID_PARAMETER;

this check will return INVALID_PARAMETER whether the variable exists
or not.

> We should later in the code check if the variable already exists and if
> it is volatile return EFI_WRITE_PROTECTED.
> 
> Best regards
> 
> Heinrich
> 
> >
> >> +  if ((attributes & ~(u32)EFI_VARIABLE_MASK))
> >> +  return EFI_INVALID_PARAMETER;
> >> +
> >> +  return efi_set_variable_rt_int(variable_name, vendor,

Re: [PATCH 15/16] efi_loader: enable UEFI variables at runtime

2020-04-01 Thread Heinrich Schuchardt 
On 4/1/20 3:41 AM, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
>> Enable UEFI variables at runtime.
>>
>> Signed-off-by: Heinrich Schuchardt 
>> ---
>>  lib/efi_loader/efi_runtime.c  |  6 +-
>>  lib/efi_loader/efi_variable.c | 23 +--
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>> index 664a0422e2..acd644202d 100644
>> --- a/lib/efi_loader/efi_runtime.c
>> +++ b/lib/efi_loader/efi_runtime.c
>> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
>>  rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
>>  rt_table->length = sizeof(struct efi_rt_properties_table);
>>  rt_table->runtime_services_supported =
>> +EFI_RT_SUPPORTED_GET_VARIABLE |
>> +EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
>> +EFI_RT_SUPPORTED_SET_VARIABLE |
>>  EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
>> -EFI_RT_SUPPORTED_CONVERT_POINTER;
>> +EFI_RT_SUPPORTED_CONVERT_POINTER |
>> +EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
>>
>>  /*
>>   * This value must be synced with efi_runtime_detach_list
>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>> index 7c39542968..cf8b44c535 100644
>> --- a/lib/efi_loader/efi_variable.c
>> +++ b/lib/efi_loader/efi_variable.c
>> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI 
>> efi_query_variable_info(
>>  u64 *remaining_variable_storage_size,
>>  u64 *maximum_variable_size)
>>  {
>> -/*
>>  *maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
>>   sizeof(struct efi_var_file);
>>  *remaining_variable_storage_size = efi_var_mem_free();
>>  *maximum_variable_size = EFI_VAR_BUF_SIZE -
>>   sizeof(struct efi_var_file) -
>>   sizeof(struct efi_var_entry);
>> -*/
>> -return EFI_UNSUPPORTED;
>> +return EFI_SUCCESS;
>>  }
>>
>>  /**
>> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
>>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>>   u32 *attributes, efi_uintn_t *data_size, void *data)
>>  {
>> -return EFI_UNSUPPORTED;
>> +return efi_get_variable_int(variable_name, vendor, attributes,
>> +data_size, data);
>>  }
>>
>>  /**
>> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
>>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>> u16 *variable_name, efi_guid_t *vendor)
>>  {
>> -return EFI_UNSUPPORTED;
>> +return efi_get_next_variable_name_int(variable_name_size, variable_name,
>> +  vendor);
>>  }
>>
>>  /**
>> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const 
>> efi_guid_t *vendor,
>>   u32 attributes, efi_uintn_t data_size,
>>   const void *data)
>>  {
>> -return EFI_UNSUPPORTED;
>> +const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
>> +EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +EFI_VARIABLE_RUNTIME_ACCESS;
>> +
>
> Why not support APPEND_WRITE?

This is the list of attributes that have to be set as a *minimum* in a
valid call to SetVariable. APPEND_WRITE is not required but optional.

>
>
>> +if (attributes &&
>> +(attributes & required_attributes) != required_attributes)
>> +return EFI_INVALID_PARAMETER;
>
> UEFI specification says,
> ===8<
> Variables that have runtime access but that are not nonvolatile are read-
> only data variables once ExitBootServices() is performed.
> [snip]
> EFI_WRITE_PROTECTED The variable in question is read-only.
> ===>8===
>
> So in this case, we should return WRITE_PROTECTED instead of
> INVALID_PARAMETER.

Here I check the parameters of the call which may create a new variable.

We should later in the code check if the variable already exists and if
it is volatile return EFI_WRITE_PROTECTED.

Best regards

Heinrich

>
>> +if ((attributes & ~(u32)EFI_VARIABLE_MASK))
>> +return EFI_INVALID_PARAMETER;
>> +
>> +return efi_set_variable_rt_int(variable_name, vendor, attributes,
>> +   data_size, data);
>
> I didn't follow detailed logic of this function, but
> does it really return UNSUPPORTED if !FAT_WRITE?
>
> -Takahiro Akashi
>
>>  }
>>
>>  /**
>> --
>> 2.25.1
>>

Re: [PATCH 15/16] efi_loader: enable UEFI variables at runtime

2020-03-31 Thread AKASHI Takahiro 
On Tue, Mar 31, 2020 at 08:05:40AM +0200, Heinrich Schuchardt wrote:
> Enable UEFI variables at runtime.
> 
> Signed-off-by: Heinrich Schuchardt 
> ---
>  lib/efi_loader/efi_runtime.c  |  6 +-
>  lib/efi_loader/efi_variable.c | 23 +--
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 664a0422e2..acd644202d 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
>   rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
>   rt_table->length = sizeof(struct efi_rt_properties_table);
>   rt_table->runtime_services_supported =
> + EFI_RT_SUPPORTED_GET_VARIABLE |
> + EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
> + EFI_RT_SUPPORTED_SET_VARIABLE |
>   EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> - EFI_RT_SUPPORTED_CONVERT_POINTER;
> + EFI_RT_SUPPORTED_CONVERT_POINTER |
> + EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> 
>   /*
>* This value must be synced with efi_runtime_detach_list
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 7c39542968..cf8b44c535 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI 
> efi_query_variable_info(
>   u64 *remaining_variable_storage_size,
>   u64 *maximum_variable_size)
>  {
> - /*
>   *maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
>sizeof(struct efi_var_file);
>   *remaining_variable_storage_size = efi_var_mem_free();
>   *maximum_variable_size = EFI_VAR_BUF_SIZE -
>sizeof(struct efi_var_file) -
>sizeof(struct efi_var_entry);
> - */
> - return EFI_UNSUPPORTED;
> + return EFI_SUCCESS;
>  }
> 
>  /**
> @@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
>  efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
>u32 *attributes, efi_uintn_t *data_size, void *data)
>  {
> - return EFI_UNSUPPORTED;
> + return efi_get_variable_int(variable_name, vendor, attributes,
> + data_size, data);
>  }
> 
>  /**
> @@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
>  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>  u16 *variable_name, efi_guid_t *vendor)
>  {
> - return EFI_UNSUPPORTED;
> + return efi_get_next_variable_name_int(variable_name_size, variable_name,
> +   vendor);
>  }
> 
>  /**
> @@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const 
> efi_guid_t *vendor,
>u32 attributes, efi_uintn_t data_size,
>const void *data)
>  {
> - return EFI_UNSUPPORTED;
> + const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS;
> +

Why not support APPEND_WRITE?


> + if (attributes &&
> + (attributes & required_attributes) != required_attributes)
> + return EFI_INVALID_PARAMETER;

UEFI specification says,
===8<
Variables that have runtime access but that are not nonvolatile are read-
only data variables once ExitBootServices() is performed.
[snip]
EFI_WRITE_PROTECTED The variable in question is read-only.
===>8===

So in this case, we should return WRITE_PROTECTED instead of
INVALID_PARAMETER.

> + if ((attributes & ~(u32)EFI_VARIABLE_MASK))
> + return EFI_INVALID_PARAMETER;
> +
> + return efi_set_variable_rt_int(variable_name, vendor, attributes,
> +data_size, data);

I didn't follow detailed logic of this function, but
does it really return UNSUPPORTED if !FAT_WRITE?

-Takahiro Akashi

>  }
> 
>  /**
> --
> 2.25.1
>

[PATCH 15/16] efi_loader: enable UEFI variables at runtime

2020-03-31 Thread Heinrich Schuchardt 
Enable UEFI variables at runtime.

Signed-off-by: Heinrich Schuchardt 
---
 lib/efi_loader/efi_runtime.c  |  6 +-
 lib/efi_loader/efi_variable.c | 23 +--
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 664a0422e2..acd644202d 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -120,8 +120,12 @@ efi_status_t efi_init_runtime_supported(void)
rt_table->version = EFI_RT_PROPERTIES_TABLE_VERSION;
rt_table->length = sizeof(struct efi_rt_properties_table);
rt_table->runtime_services_supported =
+   EFI_RT_SUPPORTED_GET_VARIABLE |
+   EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME |
+   EFI_RT_SUPPORTED_SET_VARIABLE |
EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
-   EFI_RT_SUPPORTED_CONVERT_POINTER;
+   EFI_RT_SUPPORTED_CONVERT_POINTER |
+   EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;

/*
 * This value must be synced with efi_runtime_detach_list
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 7c39542968..cf8b44c535 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -326,15 +326,13 @@ efi_status_t __efi_runtime EFIAPI efi_query_variable_info(
u64 *remaining_variable_storage_size,
u64 *maximum_variable_size)
 {
-   /*
*maximum_variable_storage_size = EFI_VAR_BUF_SIZE -
 sizeof(struct efi_var_file);
*remaining_variable_storage_size = efi_var_mem_free();
*maximum_variable_size = EFI_VAR_BUF_SIZE -
 sizeof(struct efi_var_file) -
 sizeof(struct efi_var_entry);
-   */
-   return EFI_UNSUPPORTED;
+   return EFI_SUCCESS;
 }

 /**
@@ -351,7 +349,8 @@ static efi_status_t __efi_runtime EFIAPI
 efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
 u32 *attributes, efi_uintn_t *data_size, void *data)
 {
-   return EFI_UNSUPPORTED;
+   return efi_get_variable_int(variable_name, vendor, attributes,
+   data_size, data);
 }

 /**
@@ -367,7 +366,8 @@ static efi_status_t __efi_runtime EFIAPI
 efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
   u16 *variable_name, efi_guid_t *vendor)
 {
-   return EFI_UNSUPPORTED;
+   return efi_get_next_variable_name_int(variable_name_size, variable_name,
+ vendor);
 }

 /**
@@ -385,7 +385,18 @@ efi_set_variable_runtime(u16 *variable_name, const 
efi_guid_t *vendor,
 u32 attributes, efi_uintn_t data_size,
 const void *data)
 {
-   return EFI_UNSUPPORTED;
+   const u32 required_attributes = EFI_VARIABLE_NON_VOLATILE |
+   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+   EFI_VARIABLE_RUNTIME_ACCESS;
+
+   if (attributes &&
+   (attributes & required_attributes) != required_attributes)
+   return EFI_INVALID_PARAMETER;
+   if ((attributes & ~(u32)EFI_VARIABLE_MASK))
+   return EFI_INVALID_PARAMETER;
+
+   return efi_set_variable_rt_int(variable_name, vendor, attributes,
+  data_size, data);
 }

 /**
--
2.25.1