Re: [PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE
On Sat, May 09, 2020 at 11:14:51AM +0200, Heinrich Schuchardt wrote: > > + in_name_size = u16_strsize(variable_name); [...] > > The UEFI spec requires: "The size must be large enough to fit input > string supplied in VariableName buffer." > > Further it is required to return EFI_INVALID_PARAMETER if the > "Null-terminator is not found in the first VariableNameSize bytes of the > input VariableName buffer." > > Please, investigate if SMM takes care of the check or we should do it. > Smm checks for both and returns EFI_ACCESS_DENIED. In any case I don't suggest convoluting this with extra UEFI spec requirements. Variables are delegated into Smm for handling and it should handle *everything*. Any bugs/missing corner cases we end up discovering should be fixed directly into EDK2 and not apply random fixups here. This is an API to Smm and that's all it should ever do. Regards /Ilias
Re: [PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE
Hi Heinrich,
On Sat, May 09, 2020 at 11:14:51AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
> > StandAloneMM is responsible for the UEFI variable support. In
> > combination with OP-TEE and it's U-Boot supplicant, variables are
> > authenticated/validated in secure world and stored on an RPMB partition.
> >
> > So let's add a new config option in U-Boot implementing the necessary
> > calls to OP-TEE for the variable management.
> >
> > Signed-off-by: Ilias Apalodimas
> > Signed-off-by: Pipat Methavanitpong
> > Signed-off-by: Sughosh Ganu
> > ---
> > lib/efi_loader/Kconfig| 9 +
> > lib/efi_loader/Makefile | 4 +
> > lib/efi_loader/efi_variable_tee.c | 645 ++
> > 3 files changed, 658 insertions(+)
> > create mode 100644 lib/efi_loader/efi_variable_tee.c
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 1cfa24ffcf72..e385cd0b9dae 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
> > it is signed with a trusted key. To do that, you need to install,
> > at least, PK, KEK and db.
> >
> > +config EFI_MM_COMM_TEE
> > + bool "UEFI variables storage service via OP-TEE"
> > + depends on SUPPORT_EMMC_RPMB
> > + default n
> > + help
> > + If OP-TEE is present and running StandAloneMM dispatch all UEFI
> > variable
> > + related operations to that. The application will verify, authenticate
> > and
> > + store the variables on an RPMB
> > +
> > endif
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index eff3c25ec301..dba652121eab 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -34,7 +34,11 @@ obj-y += efi_root_node.o
> > obj-y += efi_runtime.o
> > obj-y += efi_setup.o
> > obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> > +ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> > +obj-y += efi_variable_tee.o
> > +else
> > obj-y += efi_variable.o
> > +endif
> > obj-y += efi_watchdog.o
> > obj-$(CONFIG_LCD) += efi_gop.o
> > obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> > diff --git a/lib/efi_loader/efi_variable_tee.c
> > b/lib/efi_loader/efi_variable_tee.c
> > new file mode 100644
> > index ..d4e206e22073
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -0,0 +1,645 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * EFI variable service via OP-TEE
> > + *
> > + * Copyright (C) 2019 Linaro Ltd.
> > + * Copyright (C) 2019 Linaro Ltd.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define PTA_STMM_CMDID_COMMUNICATE 0
>
> This seems to be a part of the OP-TEE communication interface to SMM and
> should be in an include with a comment describing the meaning of the
> constant.
Ok
>
> > +#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
> > + 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
> > +
> > +#define EFI_MM_VARIABLE_GUID \
> > + EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
> > +0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
>
> These two GUIDs are just the same by value. Looking at EFI_GUID() and
> tee_optee_ta_uuid_to_octets() it seems that TEE is using big endian
> GUIDs while UEFI uses low endian ones. I think this deserves a comment
> in your code.
>
> I would prefer to see the GUIDs in the includes. Anyway you copied from
> MdeModulePkg/Include/Protocol/SmmVariable.h
Fair enough. The reasopn we had two discrete header files was because we
followed the EDK2 example. Your recoomendation makes sense though, we'll move
everything to a single header file with sufficient commenting and add these
values as well.
>
> > +
> > +static efi_uintn_t max_buffer_size;/* comm + var + func + data */
> > +static efi_uintn_t max_payload_size; /* func + data */
> > +
> > +struct mm_connection {
> > + struct udevice *tee;
> > + u32 session;
> > +};
> > +
> > +int get_connection(struct mm_connection *conn)
>
> This function is only used locally. Please, mark it as static.
>
> Please, comment your functions according to
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
>
Will do
> > +{
> > + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
[...]
> > + *
> > + * @comm_buf: Locally allocted communcation buffer
>
> %s/allocted/allocated/
>
> > + * @dsize: Buffer size
> > + * Return: status code
>
> Please, be consistent in the capitalization of the argument descriptions.
>
Ok
> > + */
>
> %s/cmonnucation/communication/
>
> > + * it to OP-TEE
> > + *
> > + * @comm_buf: Locally allocted communcation buffer
>
> %s/allocted/allocated/
>
> You can use
Re: [PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE
On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
> StandAloneMM is responsible for the UEFI variable support. In
> combination with OP-TEE and it's U-Boot supplicant, variables are
> authenticated/validated in secure world and stored on an RPMB partition.
>
> So let's add a new config option in U-Boot implementing the necessary
> calls to OP-TEE for the variable management.
>
> Signed-off-by: Ilias Apalodimas
> Signed-off-by: Pipat Methavanitpong
> Signed-off-by: Sughosh Ganu
> ---
> lib/efi_loader/Kconfig| 9 +
> lib/efi_loader/Makefile | 4 +
> lib/efi_loader/efi_variable_tee.c | 645 ++
> 3 files changed, 658 insertions(+)
> create mode 100644 lib/efi_loader/efi_variable_tee.c
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 1cfa24ffcf72..e385cd0b9dae 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
> it is signed with a trusted key. To do that, you need to install,
> at least, PK, KEK and db.
>
> +config EFI_MM_COMM_TEE
> + bool "UEFI variables storage service via OP-TEE"
> + depends on SUPPORT_EMMC_RPMB
> + default n
> + help
> + If OP-TEE is present and running StandAloneMM dispatch all UEFI
> variable
> + related operations to that. The application will verify, authenticate
> and
> + store the variables on an RPMB
> +
> endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index eff3c25ec301..dba652121eab 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -34,7 +34,11 @@ obj-y += efi_root_node.o
> obj-y += efi_runtime.o
> obj-y += efi_setup.o
> obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> +ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> +obj-y += efi_variable_tee.o
> +else
> obj-y += efi_variable.o
> +endif
> obj-y += efi_watchdog.o
> obj-$(CONFIG_LCD) += efi_gop.o
> obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> diff --git a/lib/efi_loader/efi_variable_tee.c
> b/lib/efi_loader/efi_variable_tee.c
> new file mode 100644
> index ..d4e206e22073
> --- /dev/null
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * EFI variable service via OP-TEE
> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PTA_STMM_CMDID_COMMUNICATE 0
This seems to be a part of the OP-TEE communication interface to SMM and
should be in an include with a comment describing the meaning of the
constant.
> +#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
> + 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
> +
> +#define EFI_MM_VARIABLE_GUID \
> + EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
> + 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
These two GUIDs are just the same by value. Looking at EFI_GUID() and
tee_optee_ta_uuid_to_octets() it seems that TEE is using big endian
GUIDs while UEFI uses low endian ones. I think this deserves a comment
in your code.
I would prefer to see the GUIDs in the includes. Anyway you copied from
MdeModulePkg/Include/Protocol/SmmVariable.h
> +
> +static efi_uintn_t max_buffer_size; /* comm + var + func + data */
> +static efi_uintn_t max_payload_size; /* func + data */
> +
> +struct mm_connection {
> + struct udevice *tee;
> + u32 session;
> +};
> +
> +int get_connection(struct mm_connection *conn)
This function is only used locally. Please, mark it as static.
Please, comment your functions according to
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> +{
> + static const struct tee_optee_ta_uuid uuid = PTA_STMM_UUID;
> + struct udevice *tee = NULL;
> + struct tee_open_session_arg arg;
> + int rc;
> +
> + tee = tee_find_device(tee, NULL, NULL, NULL);
> + if (!tee)
> + return -ENODEV;
> +
> + memset(, 0, sizeof(arg));
> + tee_optee_ta_uuid_to_octets(arg.uuid, );
> + rc = tee_open_session(tee, , 0, NULL);
> + if (!rc) {
> + conn->tee = tee;
> + conn->session = arg.session;
> + }
> +
> + return rc;
> +}
> +
> +/**
> + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
> + *
> + * @comm_buf:Locally allocted communcation buffer
%s/allocted/allocated/
> + * @dsize: Buffer size
> + * Return: status code
Please, be consistent in the capitalization of the argument descriptions.
> + */
> +static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
> +{
> + ulong buf_size;
> + efi_status_t ret;
> + struct mm_communicate_header *mm_hdr;
> + struct mm_connection conn = { NULL, 0 };
> + struct
