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.aka...@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';
> >
> >
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
