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

From: Ard Biesheuvel <ardb@kernel.org>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: linux-efi <linux-efi@vger.kernel.org>
Subject: Re: [PATCH 00/24] efi/libstub: Add printf implementation
Date: Tue, 19 May 2020 18:44:27 +0200	[thread overview]
Message-ID: <CAMj1kXGuM6sEE5HoAmGP7EZDnyUsW73FH+f2C_MvH76UCMYkpg@mail.gmail.com> (raw)
In-Reply-To: <20200519150629.GB1526900@rani.riverdale.lan>

On Tue, 19 May 2020 at 17:06, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> 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.
>

Anything that will make it more robust is good to have.

> 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?
>

Not that I am aware of.

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

Someone else may have thought of that already, so we can't simply
start treating anything past the first newline or \0 as part of the
command line.