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

[Ilias Apalodimas]

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)


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

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
> >
> >