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