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

On Thu, Feb 09, 2023 at 05:23:02PM +0100, Ard Biesheuvel wrote:

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

quoted

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

quoted

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?

Yeah, this was just commentary. I think the patch is fine as-is.

-- 
Kees Cook

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