On Tue, 11 Feb 2025 at 11:01, Mark Kettenis <mark.kette...@xs4all.nl> wrote:
>
> > From: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > Date: Tue, 11 Feb 2025 10:50:01 +0000
> >
> > Hi Mark,
> >
> > On Tue, 11 Feb 2025 at 10:17, Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> > >
> > > > From: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > > >
> > > > On EFI runtime services, we manage to preserve string literals
> > > > by placing the .efi_runtime section just before .data and preserving
> > > > it when fixing up the runtime memory by marking surrounding boottime
> > > > code as runtime. This is ok for now but will break if we update any
> > > > linker scripts and decouple .text and .runtime sections.
> > > >
> > > > So let's define the strings we used to compare in the stack for
> > > > runtime services
> > >
> > > I don't see how this helps you. Those string literals still need to
> > > be copied into the stack variable from somewhere and I don't think
> > > that with this changes where that source string literal lives...
> > >
> > > I think you need toput the string in a global variable with an
> > > appropriate __attribute__ that puts it into .runtime.
> >
> > The asm I am seeing is setting this up on the stack
> >
> > c29c0: d2800ac1 mov x1, #0x56 //
> > #86
> > {
> > c29c4: a9ba7bfd stp x29, x30, [sp, #-96]!
> > const u16 t[] = u"VarToFile";
> > c29c8: f2a00c21 movk x1, #0x61, lsl #16
> > c29cc: f2c00e41 movk x1, #0x72, lsl #32
> > {
> > c29d0: 910003fd mov x29, sp
> > const u16 t[] = u"VarToFile";
> > c29d4: f2e00a81 movk x1, #0x54, lsl #48
> > c29d8: f90027e1 str x1, [sp, #72]
> > c29dc: d2800de1 mov x1, #0x6f //
> > #111
> > {
> > c29e0: a90153f3 stp x19, x20, [sp, #16]
> > const u16 t[] = u"VarToFile";
> > c29e4: f2a008c1 movk x1, #0x46, lsl #16
> > c29e8: f2c00d21 movk x1, #0x69, lsl #32
> > {
> > c29ec: a9025bf5 stp x21, x22, [sp, #32]
> > const u16 t[] = u"VarToFile";
> > c29f0: f2e00d81 movk x1, #0x6c, lsl #48
> > {
> > c29f4: f9001bf7 str x23, [sp, #48]
> > const u16 t[] = u"VarToFile";
> > c29f8: f9002be1 str x1, [sp, #80]
> > c29fc: 52800ca1 mov w1, #0x65 //
> > #101
> > c2a00: b9005be1 str w1, [sp, #88]
> > if (!variable_name || !vendor || !data_size)
> >
>
> Oh clever! It basically uses a bunch of mov instructions to create
> the string on the stack.
Yep, but this is just me saying "I tested it before posting it!".
We do need to make this arch agnostic.
> > While the older code is doing
> >
> > if (!u16_strcmp(variable_name, u"VarToFile"))
> > a88: 90000001 adrp x1, 0 <__image_copy_start>
> >
> >
> > But I think that's very arm and compiler-specific, so I'll change it
> > to what you and Heinrich suggested
>
> Possibly even depends on the optimization options you use.
PIE perhaps?
Anyway. I'll wait a day or two in case anyone else has comments and send a v2
Thanks for taking a look
/Ilias
> And it
> would almost certainly not do this if the string was longer. So yes,
> Heinrich's suggestion is probably the way to go.
>
> >
> > Thanks
> > /Ilias
> > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > > > ---
> > > > lib/efi_loader/efi_var_mem.c | 3 ++-
> > > > lib/efi_loader/efi_variable_tee.c | 3 ++-
> > > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> > > > index b265d95dd6ba..985e0baa128d 100644
> > > > --- a/lib/efi_loader/efi_var_mem.c
> > > > +++ b/lib/efi_loader/efi_var_mem.c
> > > > @@ -310,6 +310,7 @@ efi_get_variable_mem(const u16 *variable_name,
> > > > const efi_guid_t *vendor,
> > > > {
> > > > efi_uintn_t old_size;
> > > > struct efi_var_entry *var;
> > > > + u16 vtf[] = u"VarToFile";
> > > > u16 *pdata;
> > > >
> > > > if (!variable_name || !vendor || !data_size)
> > > > @@ -331,7 +332,7 @@ efi_get_variable_mem(const u16 *variable_name,
> > > > const efi_guid_t *vendor,
> > > > if (timep)
> > > > *timep = var->time;
> > > >
> > > > - if (!u16_strcmp(variable_name, u"VarToFile"))
> > > > + if (!u16_strcmp(variable_name, vtf))
> > > > return efi_var_collect_mem(data, data_size,
> > > > EFI_VARIABLE_NON_VOLATILE);
> > > >
> > > > old_size = *data_size;
> > > > diff --git a/lib/efi_loader/efi_variable_tee.c
> > > > b/lib/efi_loader/efi_variable_tee.c
> > > > index 0d090d051dd4..8d173e58d2f7 100644
> > > > --- a/lib/efi_loader/efi_variable_tee.c
> > > > +++ b/lib/efi_loader/efi_variable_tee.c
> > > > @@ -780,6 +780,7 @@ efi_status_t efi_set_variable_int(const u16
> > > > *variable_name,
> > > > efi_uintn_t payload_size;
> > > > efi_uintn_t name_size;
> > > > u8 *comm_buf = NULL;
> > > > + u16 pk[] = u"PK";
> > > > bool ro;
> > > >
> > > > if (!variable_name || variable_name[0] == 0 || !vendor) {
> > > > @@ -858,7 +859,7 @@ efi_status_t efi_set_variable_int(const u16
> > > > *variable_name,
> > > > if (alt_ret != EFI_SUCCESS)
> > > > goto out;
> > > >
> > > > - if (!u16_strcmp(variable_name, u"PK"))
> > > > + if (!u16_strcmp(variable_name, pk))
> > > > alt_ret = efi_init_secure_state();
> > > > out:
> > > > free(comm_buf);
> > > > --
> > > > 2.43.0
> > > >
> > > >
> >
