Re: [U-Boot] [PATCH v2 1/2] efi_loader: variable: support APPEND_WRITE

On Mon, Sep 16, 2019 at 07:03:41PM +0200, Heinrich Schuchardt wrote:

On 9/6/19 8:09 AM, AKASHI Takahiro wrote:

...

If EFI_VARIABLE_APPEND_WRITE is specified in attributes at efi_set_variable(), specified data will be appended to the variable's original value. Attributes other than APPEND_WRITE should not be modified.

With this patch, APPEND_WRITE test in 'variables' selftest will pass.

With both patches of the series applied I get the following warning in bootefi selftest:

lib/efi_selftest/efi_selftest_variables.c(151): TODO: SetVariable(APPEND_WRITE) to read-only variable returns wrong code

Yes, I know. That's why I used TODO here. The reason that this test fails is that we don't support read-only variables, more strictly, we do have read-only attribute, but don't have any interface to specify read-only attribute for any variable.

Read-only is an implicit nature of variable only attributed to some "global" variables UEFI specification defines. So we should fix the issue in a separate patch.

Thanks, -Takahiro Akashi

Best regards

Heinrich

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 6687b69a400d..48ee255f879b 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -424,17 +424,17 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, efi_uintn_t data_size, const void *data) { char *native_name = NULL, *val = NULL, *s;

• const char *old_val;

• size_t old_size; efi_status_t ret = EFI_SUCCESS; u32 attr;

  EFI_ENTRY(""%ls" %pUl %x %zu %p", variable_name, vendor, attributes, data_size, data);

• /* TODO: implement APPEND_WRITE */ if (!variable_name || !*variable_name || !vendor || ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&

•     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) ||
  

•    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
  

•     !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))) {
  

  ret = EFI_INVALID_PARAMETER; goto out; }

@@ -445,35 +445,51 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

#define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)

• if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {

• /* delete the variable: */
  

• env_set(native_name, NULL);
  

• ret = EFI_SUCCESS;
  

• goto out;
  

• }

• old_val = env_get(native_name);
• if (old_val) {

• old_val = parse_attr(old_val, &attr);
  
  

• val = env_get(native_name);
• if (val) {

• parse_attr(val, &attr);
  

• /* We should not free val */
  

• val = NULL;
  

• /* check read-only first */
  

  if (attr & READ_ONLY) { ret = EFI_WRITE_PROTECTED; goto out; }

• /*
  

•  * attributes won't be changed
  

•  * TODO: take care of APPEND_WRITE once supported
  

•  */
  

• if (attr != attributes) {
  

• if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
  

• 	/* delete the variable: */
  

• 	env_set(native_name, NULL);
  

• 	ret = EFI_SUCCESS;
  

• 	goto out;
  

• }
  

• /* attributes won't be changed */
  

• if (attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) {
  ret = EFI_INVALID_PARAMETER;
  goto out;
  

  }

• if (attributes & EFI_VARIABLE_APPEND_WRITE) {
  

• 	if (!prefix(old_val, "(blob)")) {
  

• 		return EFI_DEVICE_ERROR;
  

• 		goto out;
  

• 	}
  

• 	old_size = strlen(old_val);
  

• } else {
  

• 	old_size = 0;
  

• }
  

• } else {

• if ((data_size == 0) || !(attributes & ACCESS_ATTR) ||
  

•     (attributes & EFI_VARIABLE_APPEND_WRITE)) {
  

• 	/* delete, but nothing to do */
  

• 	ret = EFI_NOT_FOUND;
  

• 	goto out;
  

• }
  

• old_size = 0;
  

  }

• val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);

• val = malloc(old_size + 2 * data_size

•      + strlen("{ro,run,boot,nv}(blob)") + 1);
  

  if (!val) { ret = EFI_OUT_OF_RESOURCES; goto out;

@@ -481,10 +497,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,

s = val;

• /*

• * store attributes
  

• * TODO: several attributes are not supported
  

• */
  

• /* store attributes */ attributes &= (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS);

@@ -505,8 +518,13 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, } s += sprintf(s, "}");

• if (old_size)

• /* APPEND_WRITE */
  

• s += sprintf(s, old_val);
  

• else

• s += sprintf(s, "(blob)");
  

• /* store payload: */

• s += sprintf(s, "(blob)"); s = bin2hex(s, data, data_size); *s = '\0';