Re: [PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

On Thu, 9 Feb 2023 at 17:13, Kees Cook [off-list ref] wrote:

On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:

quoted

On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:

quoted

On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:

quoted

On 2/6/23 04:49, Ard Biesheuvel wrote:

quoted

--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c

@@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)

        apm_irq_save(flags);
        firmware_restrict_branch_speculation_start();
-       ibt = ibt_save();
+       ibt = ibt_save(true);

My only nit with these is the bare use of 'true'/'false'.  It's
impossible to tell at the call-site what the 'true' means.  So, if you
happen to respin these and see a nice way to remedy this I'd appreciate it.

I've often wished for a named argument extention to C, much like named
initializers, such that one can write:

    ibt_save(.disable = true);

Because the thing you mention is very common with boolean arguments, the
what gets lost in the argument name and true/false just isn't very
telling.

But yeah, even if by some miracle all compiler guys were like, YES! and
implemented it tomorrow, we couldn't use it for a good few years anyway
:-/

Well... ;)

| [mark@lakrids:~]% cat args.c
| #include <stdbool.h>
| #include <stdio.h>
|
| struct foo_args {
|     bool enable;
|     unsigned long other;
| };
|
| void __foo(struct foo_args args)
| {
|     printf("foo:\n"
|            "  enable: %s\n"
|            "  other: 0x%lx\n",
|            args.enable ? "YES" : "NO",
|            args.other);
| }
|
| #define foo(args...) \
|     __foo((struct foo_args) { args })
|
|
| int main(int argc, char *argv[])
| {
|     foo(true);
|     foo(.enable = true);
|     foo(false, .other=0xdead);
| }
| [mark@lakrids:~]% gcc args.c -o args
| [mark@lakrids:~]% ./args
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: NO
|   other: 0xdead

I am horrified and delighted.

+1

And the resulting codegen is identical:
https://godbolt.org/z/eKTMPYc17

Without this fancy solution, what I'd seen is just using an enum:

enum do_the_thing {
        THING_DISABLE = 0,
        THING_ENABLE,
};

void foo(enum do_the_thing enable)
{
        if (enable) { ... }
}

foo(THING_ENABLE);

I have no strong preference one way or the other, but given that
apm_32.c is not the epicenter of new development, and the call from
EFI code is self-documenting already ('
ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
patch as-is, and leave it to whoever feels inclined to spend more free
time on this to come up with some nice polish to put on top.

Unless anyone minds?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel