Re: [PATCH 3/3] efi/libstub: consider CONFIG_CMDLINE for initrd= and dtb= options

[Jonathan Marek <jonathan@marek.ca>]

On 10/12/24 9:36 AM, Ard Biesheuvel wrote:
> On Sat, 12 Oct 2024 at 14:04, Jonathan Marek <jonathan@marek.ca> wrote:
>>
>>
>>
>> On 10/12/24 3:54 AM, Ard Biesheuvel wrote:
>>> On Sat, 12 Oct 2024 at 00:52, Jonathan Marek <jonathan@marek.ca> wrote:
>>>>
>>>> Replace cmdline with CONFIG_CMDLINE when it should be used instead of
>>>> load_options.
>>>>
>>>> In the EXTEND case, it may be necessary to combine both CONFIG_CMDLINE and
>>>> load_options. In that case, keep the old behavior and print a warning about
>>>> the incorrect behavior.
>>>>
>>>
>>> The core kernel has its own handling for EXTEND/FORCE, so while we
>>> should parse it in the EFI stub to look for options that affect the
>>> stub's own behavior, we should not copy it into the command line that
>>> the stub provides to the core kernel.
>>>
>>> E.g., drivers/of/fdt.c takes the bootargs from the DT and combines
>>> them with CONFIG_CMDLINE.
>>>
>>>
>>
>> I'm aware of that - the replacement the commit message is referring to
>> is specifically for handle_cmdline_files() which this commit is modifying.
>>
> 
> Ah ok - I missed that.
> 
> This is the kind of context that I'd expect in a cover letter, i.e.,
> that the command line handling is inconsistent, and that we obtain the
> command line from the loaded image twice.
> 
> Also, the fact the initrd= handling and dtb= are special, because
> a) multiple initrd=  arguments are processed in order, and the files
> concatenated,
> b) the filenames are consumed as UTF-16 as they are plugged into the
> file I/O protocols
> 

(not relevant to this commit, but I need to say that concatenating dtb 
files makes no sense, only the first one will be used by the kernel)

>> Currently efistub completely ignores initrd= and dtb= options provided
>> through CONFIG_CMDLINE (handle_cmdline_files() only parses the EFI options)
>>
> 
> Indeed. You haven't explained why this is a problem. initrd= and dtb=
> contain references to files in the file system, and this does not seem
> like something CONFIG_EXTEND was intended for.
> 

Its not the expected/documented behavior, that should be enough to make 
it a problem. Nowhere is it documented that these options would be 
ignored if provided through CONFIG_CMDLINE.

>> For the EXTEND case, I didn't implement the full solution because its
>> more complex and EXTEND is not available on arm64 anyway, so I went with
>> just printing a warning instead.
> 
> This code is shared between all architectures, so what arm64 does or
> does not support is irrelevant.
> 
> Can you explain your use case please?
> 

I boot linux as the "EFI/Boot/bootaa64.efi" on my EFI partition. The 
firmware does not provide any load options. This system needs a dtb, so 
I add the dtb to my EFI partition and configure it using the dtb= option 
(using CONFIG_CMDLINE).