On Fri, Jan 6, 2017 at 8:28 AM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 6 January 2017 at 10:16, Lukas Wunner <lukas@xxxxxxxxx> wrote:
>> On Wed, Jan 04, 2017 at 05:38:13PM +0000, Ard Biesheuvel wrote:
>>> On 2 January 2017 at 10:24, Lukas Wunner <lukas@xxxxxxxxx> wrote:
>>> > GUIDs and variable names passed to GetVariable() / SetVariable() are not
>>> > modified, so declare them const. This allows the compiler to place them
>>> > in the rodata section instead of generating them on the stack at
>>> > runtime. Pass GUIDs as literals rather than assigning them to temporary
>>> > variables to save a bit on code.
>>> >
>>> > Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
>>> > Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>>> > Cc: Tony Luck <tony.luck@xxxxxxxxx>
>>> > Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
>>> > Cc: Anton Vorontsov <anton@xxxxxxxxxx>
>>> > Cc: Colin Cross <ccross@xxxxxxxxxxx>
>>> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>>> > Cc: Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx>
>>> > Cc: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
>>> > Cc: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
>>> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
>>> > ---
>>> > arch/ia64/kernel/efi.c | 15 ++++-----------
>>> > arch/x86/platform/efi/quirks.c | 2 +-
>>> > drivers/firmware/efi/efi-pstore.c | 4 ++--
>>> > drivers/firmware/efi/efibc.c | 3 +--
>>> > drivers/firmware/efi/libstub/arm-stub.c | 5 ++---
>>> > drivers/infiniband/hw/hfi1/efivar.c | 6 +-----
>>> > drivers/scsi/isci/probe_roms.c | 2 +-
>>> > 7 files changed, 12 insertions(+), 25 deletions(-)
>>> >
>>> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>> > index f57d089..f1264ab 100644
>>> > --- a/arch/ia64/kernel/efi.c
>>> > +++ b/arch/ia64/kernel/efi.c
>>> > @@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
>>> > efi_uart_console_only(void)
>>> > {
>>> > efi_status_t status;
>>> > - char *s, name[] = "ConOut";
>>> > - efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
>>> > - efi_char16_t *utf16, name_utf16[32];
>>> > + const char name[] = "ConOut";
>>> > + const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };
>>>
>>> I know you mention 'static' in 10/10, but I don't understand why you
>>> don't make these static to begin with. That also allows you to
>>> annotate them as __initconst, which is not possible for compound
>>> initializers (including the GUID)
>>
>> Right, I hadn't thought of __initconst. In this particular case indeed
>> it might make sense to use "static const efi_guid_t guid __initconst = ...",
>> and arguably for the strings as well. But where should I draw the line?
>>
>> There are a handful of other GUIDs used in __init functions, e.g. in
>> arch/ia64/ there's ESI_TABLE_GUID in kernel/esi.c:esi_init() and
>> SAL_SYSTEM_TABLE_GUID in sn/kernel/setup.c:early_sn_setup().
>> This patch set results in them being placed in rodata, but not
>> init.rodata. Should I declare a temporary "static const __initconst"
>> variable for each of them? That would give us 16 bytes in each of the
>> two cases being freed after kernel initialization.
>>
>> However string literals aren't put in init.rodata either even though
>> they're declared in an __init function. E.g. early_sn_setup() contains
>> a string literal "failed to find SAL entry point\n". That's 31 bytes.
>> esi_init() contains even 103 bytes of string literals.
>>
>> So I'm not sure if it's worth to move the GUIDs to init.rodata (at the
>> expense of an additional LoC to declare a temporary variable) even
>> though we waste more space for string literals.
>>
>
> For the string literals I think there are some solutions possible
> involving compiler plugins. Kees?
Not yet, but coming. The "initify" plugin will move string literals to
init, IIUC. There are still some false-positive warnings generated by
modpost when initify is enabled, so I haven't push it into my -next
tree yet.
-Kees
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
