[PATCH] efi_loader: remove comparisons to string literals from runtime

[Mark Kettenis]

> From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Date: Tue, 11 Feb 2025 10:50:01 +0000
> 
> Hi Mark,
> 
> On Tue, 11 Feb 2025 at 10:17, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> >
> > > From: Ilias Apalodimas <ilias.apalodimas at 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.

> 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.  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.apalodimas at 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
> > >
> > >
>