mbox series

[v3,0/4]

Message ID 20240418125456.50944-1-ilias.apalodimas@linaro.org
Headers show
Series [v3,1/4] efi_loader: conditionally enable SetvariableRT | expand

Message

Ilias Apalodimas April 18, 2024, 12:54 p.m. UTC
Hi!
This is v3 of SetVariable at runtime [0]

Nothing changed drastically from v2.
A few more test cases have been added, comments/suggestions have been
addressed and a bug where deleting a variable by setting 'attributes' to
0 has been fixed.

Changes since v2:
- Add more selftests checking for variable deletion as well as the
  VarToFile header format
- Use unaligned sized variables on tests
- Add a new function to get the variable entry length instead of
  repurposing efi_var_mem_compare()
- Converted VarToFile to RO
- Added a few comments requested by Heinrich
- Fixed a bug where SetVariable with attributes set to 0 did not delete
  the variable but returned EFI_INVALID_PARAMETER instead
- Run FWTS 'uefitests' and attach the log in patch #1
- Added r-b tags from Heinrich

Changes since v1:
- Instead of Creating VarToFile at SetVariable, create it on GetVariable.
  This allows us to get rid of the preallocated RT buffer, since the
  address is user provided
- convert Set/GetVariableRT -> Set/GetVariable at runtime
- return EFI_INVALID_PARAM is NV is not set at runtime
- Heinrich sent me the efi_var_collect_mem() variant

Changes since the RFC:
- Return EFI_INVALID_PARAM if attributes are not volatile
- Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
- Add 2 EFI variables and allow userspace to write the file
- Add selftests

I also have a patch enable QueryVariableInfo [1], but I don't want to
introduce new patches now. This also needs a few more testcases of its
own so I'll send it once we finalize this one.

[0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
[1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0

Regards
/Ilias

Ilias Apalodimas (4):
  efi_loader: conditionally enable SetvariableRT
  efi_loader: Add OS notifications for SetVariable at runtime
  efi_loader: add an EFI variable with the file contents
  efi_selftest: add tests for setvariableRT

 include/efi_loader.h                          |   4 +
 include/efi_variable.h                        |  16 +-
 lib/charset.c                                 |   2 +-
 lib/efi_loader/Kconfig                        |  16 ++
 lib/efi_loader/efi_runtime.c                  |  42 ++++
 lib/efi_loader/efi_var_common.c               |   6 +-
 lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
 lib/efi_loader/efi_variable.c                 | 122 ++++++++--
 lib/efi_loader/efi_variable_tee.c             |   5 -
 .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
 10 files changed, 495 insertions(+), 80 deletions(-)

--
2.40.1

Comments

Heinrich Schuchardt April 20, 2024, 7:20 a.m. UTC | #1
On 4/18/24 14:54, Ilias Apalodimas wrote:
> Hi!
> This is v3 of SetVariable at runtime [0]
>
> Nothing changed drastically from v2.
> A few more test cases have been added, comments/suggestions have been
> addressed and a bug where deleting a variable by setting 'attributes' to
> 0 has been fixed.
>
> Changes since v2:
> - Add more selftests checking for variable deletion as well as the
>    VarToFile header format
> - Use unaligned sized variables on tests
> - Add a new function to get the variable entry length instead of
>    repurposing efi_var_mem_compare()
> - Converted VarToFile to RO
> - Added a few comments requested by Heinrich
> - Fixed a bug where SetVariable with attributes set to 0 did not delete
>    the variable but returned EFI_INVALID_PARAMETER instead
> - Run FWTS 'uefitests' and attach the log in patch #1
> - Added r-b tags from Heinrich
>
> Changes since v1:
> - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
>    This allows us to get rid of the preallocated RT buffer, since the
>    address is user provided
> - convert Set/GetVariableRT -> Set/GetVariable at runtime
> - return EFI_INVALID_PARAM is NV is not set at runtime
> - Heinrich sent me the efi_var_collect_mem() variant
>
> Changes since the RFC:
> - Return EFI_INVALID_PARAM if attributes are not volatile
> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> - Add 2 EFI variables and allow userspace to write the file
> - Add selftests
>
> I also have a patch enable QueryVariableInfo [1], but I don't want to
> introduce new patches now. This also needs a few more testcases of its
> own so I'll send it once we finalize this one.
>
> [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
>
> Regards
> /Ilias
>
> Ilias Apalodimas (4):
>    efi_loader: conditionally enable SetvariableRT
>    efi_loader: Add OS notifications for SetVariable at runtime
>    efi_loader: add an EFI variable with the file contents
>    efi_selftest: add tests for setvariableRT

I am missing a defconfig change which is needed to run the unit test in
Gitlab CI. I would prefer testing this on the sandbox.

Best regards

Heinrich

>
>   include/efi_loader.h                          |   4 +
>   include/efi_variable.h                        |  16 +-
>   lib/charset.c                                 |   2 +-
>   lib/efi_loader/Kconfig                        |  16 ++
>   lib/efi_loader/efi_runtime.c                  |  42 ++++
>   lib/efi_loader/efi_var_common.c               |   6 +-
>   lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
>   lib/efi_loader/efi_variable.c                 | 122 ++++++++--
>   lib/efi_loader/efi_variable_tee.c             |   5 -
>   .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
>   10 files changed, 495 insertions(+), 80 deletions(-)
>
> --
> 2.40.1
>
Ilias Apalodimas April 20, 2024, 11:57 a.m. UTC | #2
On Sat, 20 Apr 2024 at 10:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/18/24 14:54, Ilias Apalodimas wrote:
> > Hi!
> > This is v3 of SetVariable at runtime [0]
> >
> > Nothing changed drastically from v2.
> > A few more test cases have been added, comments/suggestions have been
> > addressed and a bug where deleting a variable by setting 'attributes' to
> > 0 has been fixed.
> >
> > Changes since v2:
> > - Add more selftests checking for variable deletion as well as the
> >    VarToFile header format
> > - Use unaligned sized variables on tests
> > - Add a new function to get the variable entry length instead of
> >    repurposing efi_var_mem_compare()
> > - Converted VarToFile to RO
> > - Added a few comments requested by Heinrich
> > - Fixed a bug where SetVariable with attributes set to 0 did not delete
> >    the variable but returned EFI_INVALID_PARAMETER instead
> > - Run FWTS 'uefitests' and attach the log in patch #1
> > - Added r-b tags from Heinrich
> >
> > Changes since v1:
> > - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
> >    This allows us to get rid of the preallocated RT buffer, since the
> >    address is user provided
> > - convert Set/GetVariableRT -> Set/GetVariable at runtime
> > - return EFI_INVALID_PARAM is NV is not set at runtime
> > - Heinrich sent me the efi_var_collect_mem() variant
> >
> > Changes since the RFC:
> > - Return EFI_INVALID_PARAM if attributes are not volatile
> > - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> > - Add 2 EFI variables and allow userspace to write the file
> > - Add selftests
> >
> > I also have a patch enable QueryVariableInfo [1], but I don't want to
> > introduce new patches now. This also needs a few more testcases of its
> > own so I'll send it once we finalize this one.
> >
> > [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
> >
> > Regards
> > /Ilias
> >
> > Ilias Apalodimas (4):
> >    efi_loader: conditionally enable SetvariableRT
> >    efi_loader: Add OS notifications for SetVariable at runtime
> >    efi_loader: add an EFI variable with the file contents
> >    efi_selftest: add tests for setvariableRT
>
> I am missing a defconfig change which is needed to run the unit test in
> Gitlab CI. I would prefer testing this on the sandbox.
>
> Best regards

Ok I'll send a followup since you already sent a PR with these

Thanks!
/Ilias
>
> Heinrich
>
> >
> >   include/efi_loader.h                          |   4 +
> >   include/efi_variable.h                        |  16 +-
> >   lib/charset.c                                 |   2 +-
> >   lib/efi_loader/Kconfig                        |  16 ++
> >   lib/efi_loader/efi_runtime.c                  |  42 ++++
> >   lib/efi_loader/efi_var_common.c               |   6 +-
> >   lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
> >   lib/efi_loader/efi_variable.c                 | 122 ++++++++--
> >   lib/efi_loader/efi_variable_tee.c             |   5 -
> >   .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
> >   10 files changed, 495 insertions(+), 80 deletions(-)
> >
> > --
> > 2.40.1
> >
>