| Message ID | 20190514045752.10035-1-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot] efi_loader: variable: attributes may not bechanged if a variable exists | expand |
On 5/14/19 6:57 AM, AKASHI Takahiro wrote: > If a variable already exists, efi_set_variable() should not change > the variable's attributes. This patch enforces it. This behavior is mandated by UEFI spec 2.7. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_variable.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 37728c3c165d..c4f3a5d2743d 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -450,6 +450,15 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, > ret = EFI_WRITE_PROTECTED; > goto out; > } > + > + /* > + * attributes won't be changed > + * TODO: take care of APPEND_WRITE once supported > + */ > + if (attr != attributes) { > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > } > > val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1); >
On 5/14/19 8:35 AM, Heinrich Schuchardt wrote: > On 5/14/19 6:57 AM, AKASHI Takahiro wrote: >> If a variable already exists, efi_set_variable() should not change >> the variable's attributes. This patch enforces it. > > This behavior is mandated by UEFI spec 2.7. > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> This patch let's `bootefi selftest`fail: Executing 'variables' lib/efi_selftest/efi_selftest_variables.c(60): TODO: QueryVariableInfo failed lib/efi_selftest/efi_selftest_variables.c(119): ERROR: SetVariable failed lib/efi_selftest/efi_selftest.c(110): ERROR: Executing 'variables' failed The preferred solution would be to implement APPEND_WRITE. Otherwise at least adjust the unit test concerning APPEND_WRITE to use efi_st_todo() and not to abort the test. I suggest that you always run `bootefi selftest` before submitting changes to the UEFI sub-system. Best regards Heinrich > >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> lib/efi_loader/efi_variable.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/lib/efi_loader/efi_variable.c >> b/lib/efi_loader/efi_variable.c >> index 37728c3c165d..c4f3a5d2743d 100644 >> --- a/lib/efi_loader/efi_variable.c >> +++ b/lib/efi_loader/efi_variable.c >> @@ -450,6 +450,15 @@ efi_status_t EFIAPI efi_set_variable(u16 >> *variable_name, >> ret = EFI_WRITE_PROTECTED; >> goto out; >> } >> + >> + /* >> + * attributes won't be changed >> + * TODO: take care of APPEND_WRITE once supported >> + */ >> + if (attr != attributes) { >> + ret = EFI_INVALID_PARAMETER; >> + goto out; >> + } >> } >> >> val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1); >> > >
On Tue, May 14, 2019 at 08:08:49PM +0200, Heinrich Schuchardt wrote: > On 5/14/19 8:35 AM, Heinrich Schuchardt wrote: > >On 5/14/19 6:57 AM, AKASHI Takahiro wrote: > >>If a variable already exists, efi_set_variable() should not change > >>the variable's attributes. This patch enforces it. > > > >This behavior is mandated by UEFI spec 2.7. > > > >Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > This patch let's `bootefi selftest`fail: > > Executing 'variables' > lib/efi_selftest/efi_selftest_variables.c(60): > TODO: QueryVariableInfo failed > lib/efi_selftest/efi_selftest_variables.c(119): > ERROR: SetVariable failed > lib/efi_selftest/efi_selftest.c(110): > ERROR: Executing 'variables' failed > > The preferred solution would be to implement APPEND_WRITE. > > Otherwise at least adjust the unit test concerning APPEND_WRITE to use > efi_st_todo() and not to abort the test. Since the current code doesn't supoort APPEND_WRITE, my commit doesn't break anything. You should fix selftest first. I don't have an immediate plan to implement APPEND_WRITE for now. -Takahiro Akashi > I suggest that you always run `bootefi selftest` before submitting > changes to the UEFI sub-system. > > Best regards > > Heinrich > > > > >> > >>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>--- > >> lib/efi_loader/efi_variable.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >>diff --git a/lib/efi_loader/efi_variable.c > >>b/lib/efi_loader/efi_variable.c > >>index 37728c3c165d..c4f3a5d2743d 100644 > >>--- a/lib/efi_loader/efi_variable.c > >>+++ b/lib/efi_loader/efi_variable.c > >>@@ -450,6 +450,15 @@ efi_status_t EFIAPI efi_set_variable(u16 > >>*variable_name, > >> ret = EFI_WRITE_PROTECTED; > >> goto out; > >> } > >>+ > >>+ /* > >>+ * attributes won't be changed > >>+ * TODO: take care of APPEND_WRITE once supported > >>+ */ > >>+ if (attr != attributes) { > >>+ ret = EFI_INVALID_PARAMETER; > >>+ goto out; > >>+ } > >> } > >> > >> val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1); > >> > > > > >
On 5/15/19 8:09 AM, AKASHI Takahiro wrote: > On Tue, May 14, 2019 at 08:08:49PM +0200, Heinrich Schuchardt wrote: >> On 5/14/19 8:35 AM, Heinrich Schuchardt wrote: >>> On 5/14/19 6:57 AM, AKASHI Takahiro wrote: >>>> If a variable already exists, efi_set_variable() should not change >>>> the variable's attributes. This patch enforces it. >>> >>> This behavior is mandated by UEFI spec 2.7. >>> >>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> >> This patch let's `bootefi selftest`fail: >> >> Executing 'variables' >> lib/efi_selftest/efi_selftest_variables.c(60): >> TODO: QueryVariableInfo failed >> lib/efi_selftest/efi_selftest_variables.c(119): >> ERROR: SetVariable failed >> lib/efi_selftest/efi_selftest.c(110): >> ERROR: Executing 'variables' failed >> >> The preferred solution would be to implement APPEND_WRITE. >> >> Otherwise at least adjust the unit test concerning APPEND_WRITE to use >> efi_st_todo() and not to abort the test. > > Since the current code doesn't supoort APPEND_WRITE, my commit > doesn't break anything. You should fix selftest first. > > I don't have an immediate plan to implement APPEND_WRITE for now. > > -Takahiro Akashi > >> I suggest that you always run `bootefi selftest` before submitting >> changes to the UEFI sub-system. >> >> Best regards >> >> Heinrich >> >>> >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> lib/efi_loader/efi_variable.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/lib/efi_loader/efi_variable.c >>>> b/lib/efi_loader/efi_variable.c >>>> index 37728c3c165d..c4f3a5d2743d 100644 >>>> --- a/lib/efi_loader/efi_variable.c >>>> +++ b/lib/efi_loader/efi_variable.c >>>> @@ -450,6 +450,15 @@ efi_status_t EFIAPI efi_set_variable(u16 >>>> *variable_name, >>>> ret = EFI_WRITE_PROTECTED; >>>> goto out; >>>> } >>>> + >>>> + /* >>>> + * attributes won't be changed >>>> + * TODO: take care of APPEND_WRITE once supported >>>> + */ >>>> + if (attr != attributes) { >>>> + ret = EFI_INVALID_PARAMETER; >>>> + goto out; You are freeing val. But this value was not allocated by you. I saw this with `bootefi selftest` after applying diff --git a/lib/efi_selftest/efi_selftest_variables.c b/lib/efi_selftest/efi_selftest_variables.c index b028c64bbc..e8346d0d4a 100644 --- a/lib/efi_selftest/efi_selftest_variables.c +++ b/lib/efi_selftest/efi_selftest_variables.c @@ -115,10 +115,8 @@ static int execute(void) EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_APPEND_WRITE, 7, v + 8); - if (ret != EFI_SUCCESS) { - efi_st_error("SetVariable failed\n"); - return EFI_ST_FAILURE; - } + if (ret != EFI_SUCCESS) + efi_st_todo("SetVariable: append failed\n"); len = EFI_ST_MAX_DATA_SIZE; ret = runtime->get_variable(L"efi_st_var1", &guid_vendor1, &attr, &len, data); Please, run (and if necessary adjust) unit tests before submitting patches. Best regards Heinrich >>>> + } >>>> } >>>> >>>> val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1); >>>> >>> >>> >> >
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 37728c3c165d..c4f3a5d2743d 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -450,6 +450,15 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name, ret = EFI_WRITE_PROTECTED; goto out; } + + /* + * attributes won't be changed + * TODO: take care of APPEND_WRITE once supported + */ + if (attr != attributes) { + ret = EFI_INVALID_PARAMETER; + goto out; + } } val = malloc(2 * data_size + strlen("{ro,run,boot}(blob)") + 1);
If a variable already exists, efi_set_variable() should not change the variable's attributes. This patch enforces it. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_variable.c | 9 +++++++++ 1 file changed, 9 insertions(+)