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 :)
