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

[Kees Cook <keescook@chromium.org>]

On Thu, Feb 09, 2023 at 05:23:02PM +0100, Ard Biesheuvel wrote:
> On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@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@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?

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

-- 
Kees Cook