Re: [PATCH 00/24] efi/libstub: Add printf implementation

From: Arvind Sankar <nivedita@alum.mit.edu>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Arvind Sankar <nivedita@alum.mit.edu>,
	linux-efi <linux-efi@vger.kernel.org>
Subject: Re: [PATCH 00/24] efi/libstub: Add printf implementation
Date: Tue, 19 May 2020 11:06:29 -0400	[thread overview]
Message-ID: <20200519150629.GB1526900@rani.riverdale.lan> (raw)
In-Reply-To: <CAMj1kXE7EasnCLW0LrWP7f8A9dpTThSHJUP+iM-i0Tqrm=FoOA@mail.gmail.com>

On Tue, May 19, 2020 at 09:53:47AM +0200, Ard Biesheuvel wrote:
> 
> Thanks Arvind, this is looking really good!
> 
> Did you use any test code for the printf() parsing? Given that the
> kernel command line is not covered by secure boot signing (or the
> initrd, come to think of it), I'd hate to open up a security hole
> here.
> 
I only did basic functional testing, I haven't tried to actually break
it.

I think the code will be robust enough to avoid overflowing the buffer
passed to vsnprintf, even if the output ends up being garbage due to
bugs.

That said, one thing in efi_convert_cmdline is that we use int to hold
both options_chars and options_bytes. The size of load options is
limited to uint32, so int should be ok for options_chars but
options_bytes could theoretically overflow?

In any case, there's no point parsing beyond COMMAND_LINE_SIZE anyway,
so we should limit options_bytes to COMMAND_LINE_SIZE-1 + terminating
NUL, and if it's longer we can either truncate it (blindly or at
whitespace?) or ignore the options altogether. I can add that in v2.

One more question -- since the first version of the stub, we truncate
the command line at the first newline character. Do you know if there's
something that actually needs that?

efibootmgr can actually even set up the load options as a series of
NUL-terminated strings if you miss putting them all inside quotes :)