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