Message ID | 20190906060953.31481-2-takahiro.akashi@linaro.org |
---|---|
State | Accepted, archived |
Commit | 6d2f27c5fd60c1310884c873350f1edc7d831bb0 |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | efi_loader: support APPEND_WRITE | expand |
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 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'; > >
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'; > > > > >
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. > > 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)) { The understanding of EDK2 is that no access attributes means 'attributes == 0' (Function VariableServiceSetVariable() in MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c). EFI_VARIABLE_APPEND_WRITE and data_size = 0 should not delete a variable according to the UEFI spec: "Unless the EFI_VARIABLE_APPEND_WRITE, EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, or EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS attribute is set (see below), using SetVariable() with a DataSize of zero will cause the entire variable to be deleted." Best regards Heinrich > + /* 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'; > >
On Tue, Sep 17, 2019 at 10:38:47PM +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. > > > >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)) { > > The understanding of EDK2 is that no access attributes means 'attributes > == 0' (Function VariableServiceSetVariable() in > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c). No, it's not correct. The right place you should refer to is: UpdateVariable ( [snip up to l.2279 or so] // // Setting a data variable with no access, or zero DataSize attributes // causes it to be deleted. // When the EFI_VARIABLE_APPEND_WRITE attribute is set, DataSize of zero will // not delete the variable. // if ((((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0) && (DataSize == 0))|| ((Attributes & (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)) == 0)) { > EFI_VARIABLE_APPEND_WRITE and data_size = 0 should not delete a variable > according to the UEFI spec: This is correct. Thanks, -Takahiro Akashi > "Unless the EFI_VARIABLE_APPEND_WRITE, > EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, or > EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS attribute is set (see below), > using SetVariable() with a DataSize of zero will cause the entire > variable to be deleted." > > Best regards > > Heinrich > > >+ /* 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'; > > > > >
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';
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. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_variable.c | 70 ++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 26 deletions(-)