| Message ID | 20250211094942.36162-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: remove comparisons to string literals from runtime | expand |
On 11.02.25 10:49, Ilias Apalodimas wrote: > 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 > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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"; I cannot see why this change would influence the section where the string is placed. Adding a static variable marked as __efi_runtime_data should do the job. static __efi_runtime_data const u16 vtf[] = u"VarToFile"; Best regards Heinrich > 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);
> From: Ilias Apalodimas <ilias.apalodimas@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. > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 > >
Hi Mark, On Tue, 11 Feb 2025 at 10:17, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ilias Apalodimas <ilias.apalodimas@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@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 > > > >
> From: Ilias Apalodimas <ilias.apalodimas@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@xs4all.nl> wrote: > > > > > From: Ilias Apalodimas <ilias.apalodimas@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@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 > > > > > > >
On Tue, 11 Feb 2025 at 11:01, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Ilias Apalodimas <ilias.apalodimas@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@xs4all.nl> wrote: > > > > > > > From: Ilias Apalodimas <ilias.apalodimas@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.apalodimas@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 > > > > > > > > > >
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);
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 Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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(-)