On 6 January 2017 at 09:57, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> On Wed, Jan 04, 2017 at 05:54:08PM +0000, Ard Biesheuvel wrote:
>> On 2 January 2017 at 10:24, Lukas Wunner <lukas@xxxxxxxxx> wrote:
>> > The UUID_BE() and UUID_LE() macros (as well as the derived EFI_GUID())
>> > generate a compound literal for a 128-bit ID. By default, these are not
>> > stored in the rodata section but generated on the stack at runtime in a
>> > very time- and space-inefficient manner.
>>
>> This is not true if you declare those variables static const (in
>> function scope).
>>
>> In general, I think we should avoid touching uapi headers for this, so
>> I would like to explore if we can reach the same result without this
>> patch. Another objection I have to this patch is that it is context
>> dependent whether the UUID_[LE_BE]() macros in fact generate compound
>> literals: this is true for an assignment to a pointer variable, but
>> when they are used in initializers this is not the case, and so adding
>> 'const' to their definitions is not always appropriate imo.
>>
>> Are there any instances where simply turning those automatic variables
>> into static const doesn't work?
>
> The number of GUIDs used in the kernel is vast, it's many more than this
> patch set makes it appear. In a lot of cases, they're used as literals.
> I would have to declare a temporary "static const" variable for each of
> them and this patch set would become much larger and more intrusive.
> Additionally the necessity to use a static const variable instead of a
> literal seems clumsy and ugly.
>
> As an example, consider arch/ia64/kernel/mca_drv.c:mca_make_slidx() which
> contains an if-else-if ladder with 9 entries, each using a GUID literal.
> I would have expected objections on grounds of severe ugliness if I had
> changed all of these to temporary "static const" variables.
>
Yes, but we can macroize that: something like
#define __efiguid_ref(x) ({static const efi_guid_t __x = (x); __x; })
and
#define __efiguid_initref(x) ({static const __initconst efi_guid_t __x
= (x); __x; })
should do the trick I think?
> By contrast, just changing the UUID_LE() and UUID_BE() macros causes these
> literals to be placed in rodata automatically.
>
> It seems that the only reason for uuid.h to be in uapi is its usage by
> two other uapi headers, hyperv.h and mei.h. The two don't even use the
> UUID_LE() and UUID_BE() macros.
>
> The impact of adding const to these macros is limited: If the macros are
> used as an initializer in a user space application and the variable they're
> assigned to isn't declared const, gcc will silently make them non-const.
>
> Worst thing that can happen is that compiling the user space application
> results in a compiler warning. This only occurs if the UUID is passed by
> reference into a function. E.g. if this patch wasn't at the end of the
> series, these 3 warnings would occur:
>
> arch/x86/platform/efi/quirks.c: In function 'efi_delete_dummy_variable':
> arch/x86/platform/efi/quirks.c:52:35: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
> ^
> arch/x86/platform/efi/quirks.c:52:35: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> arch/x86/platform/efi/quirks.c: In function 'efi_query_variable_store':
> arch/x86/platform/efi/quirks.c:129:45: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
> ^
> arch/x86/platform/efi/quirks.c:129:45: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> drivers/scsi/isci/probe_roms.c: In function 'isci_get_efi_var':
> drivers/scsi/isci/probe_roms.c:190:8: warning: passing argument 2 of 'get_efi()->get_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> &ISCI_EFI_VENDOR_GUID,
> ^
> drivers/scsi/isci/probe_roms.c:190:8: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> This can easily be fixed in the user space application by adding "const"
> to function declarations.
>
> To reiterate what I wrote in the commit message, I don't see why anyone
> would want to modify the distinct bytes making up the UUID after declaring
> it, so marking UUIDs const seems natural.
>
> Thanks,
>
> Lukas
>
>>
>> > E.g. EFI_FILE_SYSTEM_GUID (964E5B22-6459-11D2-8E39-00A0C969723B) results
>> > in x86_64 code which requires 80 bytes text instead of 16 bytes rodata:
>> >
>> > c6 44 24 20 22 movb $0x22,0x20(%rsp)
>> > c6 44 24 21 5b movb $0x5b,0x21(%rsp)
>> > c6 44 24 22 4e movb $0x4e,0x22(%rsp)
>> > c6 44 24 23 96 movb $0x96,0x23(%rsp)
>> > c6 44 24 24 59 movb $0x59,0x24(%rsp)
>> > c6 44 24 25 64 movb $0x64,0x25(%rsp)
>> > c6 44 24 26 d2 movb $0xd2,0x26(%rsp)
>> > c6 44 24 27 11 movb $0x11,0x27(%rsp)
>> > c6 44 24 28 8e movb $0x8e,0x28(%rsp)
>> > c6 44 24 29 39 movb $0x39,0x29(%rsp)
>> > c6 44 24 2a 00 movb $0x0,0x2a(%rsp)
>> > c6 44 24 2b a0 movb $0xa0,0x2b(%rsp)
>> > c6 44 24 2c c9 movb $0xc9,0x2c(%rsp)
>> > c6 44 24 2d 69 movb $0x69,0x2d(%rsp)
>> > c6 44 24 2e 72 movb $0x72,0x2e(%rsp)
>> > c6 44 24 2f 3b movb $0x3b,0x2f(%rsp)
>> >
>> > gcc 7 improves on this by using 2x movabs + 2x mov instructions,
>> > amounting to 30 bytes text instead of 16 bytes rodata. That's more
>> > space-efficient than 80 bytes but still undesirable from a performance
>> > and security perspective:
>> >
>> > 48 b8 22 5b 4e 96 59 64 d2 11 movabs $0x11d26459964e5b,%rax
>> > 48 89 44 24 20 mov %rax,0x20(%rsp)
>> > 48 b8 8e 39 00 a0 c9 69 72 3b movabs $0x3b7269c9a00039,%rax
>> > 48 89 44 24 28 mov %rax,0x28(%rsp)
>> >
>> > The reason for generating compound literals on the stack is that they're
>> > allowed to be modified. E.g. a pointer to a UUID might be used to
>> > change some of its 16 bytes. In the context of UUIDs this seems silly
>> > though: The uuid_be, uuid_le and efi_guid_t data types hide the
>> > distinct bytes making up the ID, the bytes are not meant to be modified.
>> > Moreover, the UUIDs are usually pre-defined in a spec such as UEFI and
>> > there's no reason at all to modify them.
>> >
>> > Thus, change the UUID_BE and UUID_LE macros to declare the generated
>> > compound literal const, allowing the compiler to put it in rodata.
>> >
>> > When using the compound literal as an initializer for a variable, that
>> > variable has to be declared const as well for the UUID to become rodata.
>> >
>> > Is that all? Unfortunately not. Under certain conditions gcc refuses
>> > to store compound literals in rodata despite them being declared const.
>> > Rasmus Villemoes opened a bugzilla entry for this in December 2015:
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>> >
>> > The upshot is that if a UUID is passed by value to a function (either
>> > directly or as a variable), it is stored in rodata. This is the case
>> > for efi_guidcmp(). But if the UUID is passed by reference, it is
>> > generated on the stack, in spite of it being declared const and the
>> > function parameter being declared const. This is the case for the
>> > various EFI boot and runtime services calls such as GetVariable.
>> > A workaround is to declare a *static* const variable, assign the UUID to
>> > it and pass the variable by reference to the function.
>> >
>> > This issue also affects efi_char16_t strings which are only declared
>> > const (and not static const). They're likewise generated on the stack
>> > at runtime.
>> >
>> > clang 3.9 is not better than gcc: It does the exact opposite by putting
>> > UUIDs in rodata if they're passed by reference, and generating them on
>> > the stack if they're passed by value.
>> >
>> > Cc: Huang Ying <ying.huang@xxxxxxxxx>
>> > Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
>> > Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
>> > ---
>> > include/uapi/linux/uuid.h | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/uuid.h b/include/uapi/linux/uuid.h
>> > index 3738e5f..9184f01 100644
>> > --- a/include/uapi/linux/uuid.h
>> > +++ b/include/uapi/linux/uuid.h
>> > @@ -29,14 +29,14 @@
>> > } uuid_be;
>> >
>> > #define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
>> > -((uuid_le) \
>> > +((const uuid_le) \
>> > {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>> > (b) & 0xff, ((b) >> 8) & 0xff, \
>> > (c) & 0xff, ((c) >> 8) & 0xff, \
>> > (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
>> >
>> > #define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
>> > -((uuid_be) \
>> > +((const uuid_be) \
>> > {{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & 0xff, \
>> > ((b) >> 8) & 0xff, (b) & 0xff, \
>> > ((c) >> 8) & 0xff, (c) & 0xff, \
>> > --
>> > 2.11.0
>> >
--
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
