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

[Kees Cook <keescook@chromium.org>]

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


-- 
Kees Cook