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

[Ard Biesheuvel]

On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook at chromium.org> wrote:
>
> On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > --- 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 at 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 at lakrids:~]% gcc args.c -o args
> > | [mark at 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?