[PATCH 1/3] efi/libstub: fix efi_parse_options() ignoring the default command line

[PATCH 1/3] efi/libstub: fix efi_parse_options() ignoring the default command line

[Jonathan Marek]

efi_convert_cmdline() always returns a size of at least 1 because it counts
the NUL terminator, so the "cmdline_size == 0" condition is not possible.

Change it to compare against 1 to get the intended behavior: to use
CONFIG_CMDLINE when load_options_size is 0.

Fixes: 60f38de7a8d4 ("efi/libstub: Unify command line param parsing")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/firmware/efi/libstub/efi-stub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 958a680e0660d..709ae2d41a632 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -129,7 +129,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 
 	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
 	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
-	    cmdline_size == 0) {
+	    cmdline_size == 1) {
 		status = efi_parse_options(CONFIG_CMDLINE);
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to parse options\n");
-- 
2.45.1

[PATCH 2/3] efi/libstub: remove uneccessary cmdline_size init/check

[Jonathan Marek]

efi_convert_cmdline() always sets cmdline_size to at least 1 on success,
so both the initialization to 0 and > 0 comparison are unecessary.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/firmware/efi/libstub/efi-stub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 709ae2d41a632..f166614ef8432 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -112,7 +112,7 @@ static u32 get_supported_rt_services(void)
 
 efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 {
-	int cmdline_size = 0;
+	int cmdline_size;
 	efi_status_t status;
 	char *cmdline;
 
@@ -137,7 +137,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 		}
 	}
 
-	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_size > 0) {
+	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
 		status = efi_parse_options(cmdline);
 		if (status != EFI_SUCCESS) {
 			efi_err("Failed to parse options\n");
-- 
2.45.1

Re: [PATCH 2/3] efi/libstub: remove uneccessary cmdline_size init/check

[Ard Biesheuvel]

On Sat, 12 Oct 2024 at 00:52, Jonathan Marek <jonathan@marek.ca> wrote:
>
> efi_convert_cmdline() always sets cmdline_size to at least 1 on success,
> so both the initialization to 0 and > 0 comparison are unecessary.
>

The intent is to avoid parsing the empty string, so arguably, we
should test for cmdline_size > 1 instead. But if we fix
efi_convert_cmdline() instead, as I proposed in reply to 1/3, I guess
we can drop this patch.


> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/firmware/efi/libstub/efi-stub.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index 709ae2d41a632..f166614ef8432 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -112,7 +112,7 @@ static u32 get_supported_rt_services(void)
>
>  efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>  {
> -       int cmdline_size = 0;
> +       int cmdline_size;
>         efi_status_t status;
>         char *cmdline;
>
> @@ -137,7 +137,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>                 }
>         }
>
> -       if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_size > 0) {
> +       if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
>                 status = efi_parse_options(cmdline);
>                 if (status != EFI_SUCCESS) {
>                         efi_err("Failed to parse options\n");
> --
> 2.45.1
>

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

[Jonathan Marek]

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.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/firmware/efi/libstub/file.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index d6a025df07dcf..2a69e2b3583d4 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -208,6 +208,18 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 	if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
 		efi_chunk_size = EFI_READ_CHUNK_SIZE;
 
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
+	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
+	    cmdline_len == 0) {
+		if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_len > 0) {
+			/* both CONFIG_CMDLINE and load_options should be used */
+			efi_warn("ignoring %ls from CONFIG_CMDLINE\n", optstr);
+		} else {
+			cmdline = L"" CONFIG_CMDLINE;
+			cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
+		}
+	}
+
 	alloc_addr = alloc_size = 0;
 	do {
 		struct finfo fi;
-- 
2.45.1

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

[Ard Biesheuvel]

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.


> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/firmware/efi/libstub/file.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
> index d6a025df07dcf..2a69e2b3583d4 100644
> --- a/drivers/firmware/efi/libstub/file.c
> +++ b/drivers/firmware/efi/libstub/file.c
> @@ -208,6 +208,18 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
>         if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
>                 efi_chunk_size = EFI_READ_CHUNK_SIZE;
>
> +       if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
> +           IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
> +           cmdline_len == 0) {
> +               if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_len > 0) {
> +                       /* both CONFIG_CMDLINE and load_options should be used */
> +                       efi_warn("ignoring %ls from CONFIG_CMDLINE\n", optstr);
> +               } else {
> +                       cmdline = L"" CONFIG_CMDLINE;
> +                       cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
> +               }
> +       }
> +
>         alloc_addr = alloc_size = 0;
>         do {
>                 struct finfo fi;
> --
> 2.45.1
>

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

[Jonathan Marek]

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.

Currently efistub completely ignores initrd= and dtb= options provided 
through CONFIG_CMDLINE (handle_cmdline_files() only parses the EFI options)

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.

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

[Ard Biesheuvel]

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

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

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

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

[Jonathan Marek]

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

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

[Ard Biesheuvel]

On Sat, 12 Oct 2024 at 16:11, Jonathan Marek <jonathan@marek.ca> wrote:
>
> 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)
>

Sure, but this code was written for initrd= initially, and was reused for dtb=

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

Fair enough.

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

Right.

Would the below work for you? It's not the prettiest code in the
world, but at least it keeps the weird local to the function.

--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -189,26 +189,48 @@
                                  unsigned long *load_addr,
                                  unsigned long *load_size)
 {
-       const efi_char16_t *cmdline = efi_table_attr(image, load_options);
-       u32 cmdline_len = efi_table_attr(image, load_options_size);
        unsigned long efi_chunk_size = ULONG_MAX;
        efi_file_protocol_t *volume = NULL;
+       const efi_char16_t *cmdline;
        efi_file_protocol_t *file;
        unsigned long alloc_addr;
        unsigned long alloc_size;
        efi_status_t status;
+       bool again = false;
+       u32 cmdline_len;
        int offset;

        if (!load_addr || !load_size)
                return EFI_INVALID_PARAMETER;

-       efi_apply_loadoptions_quirk((const void **)&cmdline, &cmdline_len);
-       cmdline_len /= sizeof(*cmdline);
-
        if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
                efi_chunk_size = EFI_READ_CHUNK_SIZE;

        alloc_addr = alloc_size = 0;
+
+#ifdef CONFIG_CMDLINE
+       if (IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
+           IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ||
+           (again = (IS_ENABLED(CONFIG_X86) ||
+                     IS_ENABLED(CONFIG_CMDLINE_EXTEND)))) {
+               static const efi_char16_t builtin_cmdline[] = L""
CONFIG_CMDLINE;
+
+               cmdline = builtin_cmdline;
+               cmdline_len = sizeof(builtin_cmdline);
+       } else
+#endif
+       {
+do_load_options:
+               cmdline = efi_table_attr(image, load_options);
+               cmdline_len = efi_table_attr(image, load_options_size);
+
+               efi_apply_loadoptions_quirk((const void **)&cmdline,
+                                           &cmdline_len);
+
+               again = false;
+       }
+       cmdline_len /= sizeof(*cmdline);
+
        do {
                struct finfo fi;
                unsigned long size;
@@ -290,6 +312,9 @@
                efi_call_proto(volume, close);
        } while (offset > 0);

+       if (again)
+               goto do_load_options;
+
        *load_addr = alloc_addr;
        *load_size = alloc_size;

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

[Jonathan Marek]

On 10/12/24 10:50 AM, Ard Biesheuvel wrote:
...
> 
> Right.
> 
> Would the below work for you? It's not the prettiest code in the
> world, but at least it keeps the weird local to the function.
> 

Its missing the "load_options_size == 0" case, then CONFIG_CMDLINE 
should be used even if FORCE/etc. are not set. Otherwise it looks OK. 
(cmdline_len also shouldn't include the NUL character, but I don't think 
that matters)

> --- a/drivers/firmware/efi/libstub/file.c
> +++ b/drivers/firmware/efi/libstub/file.c
> @@ -189,26 +189,48 @@
>                                    unsigned long *load_addr,
>                                    unsigned long *load_size)
>   {
> -       const efi_char16_t *cmdline = efi_table_attr(image, load_options);
> -       u32 cmdline_len = efi_table_attr(image, load_options_size);
>          unsigned long efi_chunk_size = ULONG_MAX;
>          efi_file_protocol_t *volume = NULL;
> +       const efi_char16_t *cmdline;
>          efi_file_protocol_t *file;
>          unsigned long alloc_addr;
>          unsigned long alloc_size;
>          efi_status_t status;
> +       bool again = false;
> +       u32 cmdline_len;
>          int offset;
> 
>          if (!load_addr || !load_size)
>                  return EFI_INVALID_PARAMETER;
> 
> -       efi_apply_loadoptions_quirk((const void **)&cmdline, &cmdline_len);
> -       cmdline_len /= sizeof(*cmdline);
> -
>          if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
>                  efi_chunk_size = EFI_READ_CHUNK_SIZE;
> 
>          alloc_addr = alloc_size = 0;
> +
> +#ifdef CONFIG_CMDLINE
> +       if (IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
> +           IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ||
> +           (again = (IS_ENABLED(CONFIG_X86) ||
> +                     IS_ENABLED(CONFIG_CMDLINE_EXTEND)))) {
> +               static const efi_char16_t builtin_cmdline[] = L""
> CONFIG_CMDLINE;
> +
> +               cmdline = builtin_cmdline;
> +               cmdline_len = sizeof(builtin_cmdline);
> +       } else
> +#endif
> +       {
> +do_load_options:
> +               cmdline = efi_table_attr(image, load_options);
> +               cmdline_len = efi_table_attr(image, load_options_size);
> +
> +               efi_apply_loadoptions_quirk((const void **)&cmdline,
> +                                           &cmdline_len);
> +
> +               again = false;
> +       }
> +       cmdline_len /= sizeof(*cmdline);
> +
>          do {
>                  struct finfo fi;
>                  unsigned long size;
> @@ -290,6 +312,9 @@
>                  efi_call_proto(volume, close);
>          } while (offset > 0);
> 
> +       if (again)
> +               goto do_load_options;
> +
>          *load_addr = alloc_addr;
>          *load_size = alloc_size;
> 

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

[kernel test robot]

Hi Jonathan,

kernel test robot noticed the following build errors:

[auto build test ERROR on efi/next]
[also build test ERROR on linus/master v6.12-rc3 next-20241014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Marek/efi-libstub-remove-uneccessary-cmdline_size-init-check/20241012-065337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
patch link:    https://lore.kernel.org/r/20241011224812.25763-3-jonathan%40marek.ca
patch subject: [PATCH 3/3] efi/libstub: consider CONFIG_CMDLINE for initrd= and dtb= options
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20241015/202410150734.BTqw8f36-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410150734.BTqw8f36-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410150734.BTqw8f36-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/efi/libstub/file.c: In function 'handle_cmdline_files':
>> drivers/firmware/efi/libstub/file.c:218:38: error: expected ';' before 'CONFIG_CMDLINE'
     218 |                         cmdline = L"" CONFIG_CMDLINE;
         |                                      ^~~~~~~~~~~~~~~
         |                                      ;
   In file included from include/linux/string.h:6,
                    from include/linux/efi.h:16,
                    from drivers/firmware/efi/libstub/file.c:10:
>> drivers/firmware/efi/libstub/file.c:219:54: error: expected ')' before 'CONFIG_CMDLINE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^~~~~~~~~~~~~~
   include/linux/array_size.h:11:33: note: in definition of macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                 ^~~
   include/linux/array_size.h:11:32: note: to match this '('
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                ^
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~
>> drivers/firmware/efi/libstub/file.c:219:54: error: expected ')' before 'CONFIG_CMDLINE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^~~~~~~~~~~~~~
   include/linux/array_size.h:11:48: note: in definition of macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                ^~~
   include/linux/array_size.h:11:47: note: to match this '('
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                               ^
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~
   In file included from include/linux/init.h:5,
                    from include/linux/efi.h:15,
                    from drivers/firmware/efi/libstub/file.c:10:
>> drivers/firmware/efi/libstub/file.c:219:54: error: expected ')' before 'CONFIG_CMDLINE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^~~~~~~~~~~~~~
   include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                              ^
   include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                   ^~~~~~~~~~~
   include/linux/array_size.h:11:59: note: in expansion of macro '__must_be_array'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~
   include/linux/compiler.h:243:63: note: to match this '('
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                               ^
   include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                              ^
   include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                   ^~~~~~~~~~~
   include/linux/array_size.h:11:59: note: in expansion of macro '__must_be_array'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~
>> drivers/firmware/efi/libstub/file.c:219:54: error: expected ')' before 'CONFIG_CMDLINE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^~~~~~~~~~~~~~
   include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                              ^
   include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                   ^~~~~~~~~~~
   include/linux/array_size.h:11:59: note: in expansion of macro '__must_be_array'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~
   include/linux/compiler.h:243:69: note: to match this '('
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                                     ^
   include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
      16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
         |                                                              ^
   include/linux/compiler.h:243:51: note: in expansion of macro '__same_type'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                   ^~~~~~~~~~~
   include/linux/array_size.h:11:59: note: in expansion of macro '__must_be_array'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/file.c:219:39: note: in expansion of macro 'ARRAY_SIZE'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^~~~~~~~~~


vim +218 drivers/firmware/efi/libstub/file.c

   177	
   178	/*
   179	 * Check the cmdline for a LILO-style file= arguments.
   180	 *
   181	 * We only support loading a file from the same filesystem as
   182	 * the kernel image.
   183	 */
   184	efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
   185					  const efi_char16_t *optstr,
   186					  int optstr_size,
   187					  unsigned long soft_limit,
   188					  unsigned long hard_limit,
   189					  unsigned long *load_addr,
   190					  unsigned long *load_size)
   191	{
   192		const efi_char16_t *cmdline = efi_table_attr(image, load_options);
   193		u32 cmdline_len = efi_table_attr(image, load_options_size);
   194		unsigned long efi_chunk_size = ULONG_MAX;
   195		efi_file_protocol_t *volume = NULL;
   196		efi_file_protocol_t *file;
   197		unsigned long alloc_addr;
   198		unsigned long alloc_size;
   199		efi_status_t status;
   200		int offset;
   201	
   202		if (!load_addr || !load_size)
   203			return EFI_INVALID_PARAMETER;
   204	
   205		efi_apply_loadoptions_quirk((const void **)&cmdline, &cmdline_len);
   206		cmdline_len /= sizeof(*cmdline);
   207	
   208		if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
   209			efi_chunk_size = EFI_READ_CHUNK_SIZE;
   210	
   211		if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
   212		    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
   213		    cmdline_len == 0) {
   214			if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_len > 0) {
   215				/* both CONFIG_CMDLINE and load_options should be used */
   216				efi_warn("ignoring %ls from CONFIG_CMDLINE\n", optstr);
   217			} else {
 > 218				cmdline = L"" CONFIG_CMDLINE;
 > 219				cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Jonathan,

kernel test robot noticed the following build errors:

[auto build test ERROR on efi/next]
[also build test ERROR on linus/master v6.12-rc3 next-20241014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Marek/efi-libstub-remove-uneccessary-cmdline_size-init-check/20241012-065337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
patch link:    https://lore.kernel.org/r/20241011224812.25763-3-jonathan%40marek.ca
patch subject: [PATCH 3/3] efi/libstub: consider CONFIG_CMDLINE for initrd= and dtb= options
config: i386-defconfig (https://download.01.org/0day-ci/archive/20241015/202410150919.kDUQQNyF-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410150919.kDUQQNyF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410150919.kDUQQNyF-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/efi/libstub/file.c:218:17: error: expected ';' after expression
     218 |                         cmdline = L"" CONFIG_CMDLINE;
         |                                      ^
         |                                      ;
>> drivers/firmware/efi/libstub/file.c:218:18: error: use of undeclared identifier 'CONFIG_CMDLINE'
     218 |                         cmdline = L"" CONFIG_CMDLINE;
         |                                       ^
>> drivers/firmware/efi/libstub/file.c:219:33: error: expected ')'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^
   drivers/firmware/efi/libstub/file.c:219:18: note: to match this '('
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^
   include/linux/array_size.h:11:32: note: expanded from macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                ^
>> drivers/firmware/efi/libstub/file.c:219:33: error: expected ')'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^
   drivers/firmware/efi/libstub/file.c:219:18: note: to match this '('
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^
   include/linux/array_size.h:11:47: note: expanded from macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                               ^
>> drivers/firmware/efi/libstub/file.c:219:33: error: expected ')'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^
   drivers/firmware/efi/libstub/file.c:219:18: note: to match this '('
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^
   include/linux/array_size.h:11:59: note: expanded from macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^
   include/linux/compiler.h:243:58: note: expanded from macro '__must_be_array'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                               ^
>> drivers/firmware/efi/libstub/file.c:219:33: error: expected ')'
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                                      ^
   drivers/firmware/efi/libstub/file.c:219:18: note: to match this '('
     219 |                         cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;
         |                                       ^
   include/linux/array_size.h:11:59: note: expanded from macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                                           ^
   include/linux/compiler.h:243:64: note: expanded from macro '__must_be_array'
     243 | #define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
         |                                                                     ^
   6 errors generated.


vim +218 drivers/firmware/efi/libstub/file.c

   177	
   178	/*
   179	 * Check the cmdline for a LILO-style file= arguments.
   180	 *
   181	 * We only support loading a file from the same filesystem as
   182	 * the kernel image.
   183	 */
   184	efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
   185					  const efi_char16_t *optstr,
   186					  int optstr_size,
   187					  unsigned long soft_limit,
   188					  unsigned long hard_limit,
   189					  unsigned long *load_addr,
   190					  unsigned long *load_size)
   191	{
   192		const efi_char16_t *cmdline = efi_table_attr(image, load_options);
   193		u32 cmdline_len = efi_table_attr(image, load_options_size);
   194		unsigned long efi_chunk_size = ULONG_MAX;
   195		efi_file_protocol_t *volume = NULL;
   196		efi_file_protocol_t *file;
   197		unsigned long alloc_addr;
   198		unsigned long alloc_size;
   199		efi_status_t status;
   200		int offset;
   201	
   202		if (!load_addr || !load_size)
   203			return EFI_INVALID_PARAMETER;
   204	
   205		efi_apply_loadoptions_quirk((const void **)&cmdline, &cmdline_len);
   206		cmdline_len /= sizeof(*cmdline);
   207	
   208		if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
   209			efi_chunk_size = EFI_READ_CHUNK_SIZE;
   210	
   211		if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
   212		    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
   213		    cmdline_len == 0) {
   214			if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_len > 0) {
   215				/* both CONFIG_CMDLINE and load_options should be used */
   216				efi_warn("ignoring %ls from CONFIG_CMDLINE\n", optstr);
   217			} else {
 > 218				cmdline = L"" CONFIG_CMDLINE;
 > 219				cmdline_len = ARRAY_SIZE(L"" CONFIG_CMDLINE) - 1;

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] efi/libstub: fix efi_parse_options() ignoring the default command line

[Ard Biesheuvel]

Hi Jonathan,

Please use a cover letter when sending more than a single patch.

On Sat, 12 Oct 2024 at 00:51, Jonathan Marek <jonathan@marek.ca> wrote:
>
> efi_convert_cmdline() always returns a size of at least 1 because it counts
> the NUL terminator, so the "cmdline_size == 0" condition is not possible.
>
> Change it to compare against 1 to get the intended behavior: to use
> CONFIG_CMDLINE when load_options_size is 0.
>
> Fixes: 60f38de7a8d4 ("efi/libstub: Unify command line param parsing")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/firmware/efi/libstub/efi-stub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index 958a680e0660d..709ae2d41a632 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -129,7 +129,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>
>         if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
>             IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
> -           cmdline_size == 0) {
> +           cmdline_size == 1) {

I'd prefer it if we could keep the weirdness local to
efi_convert_cmdline(). Would the below fix things too?

--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -395,9 +395,7 @@
                }
        }

-       options_bytes++;        /* NUL termination */
-
-       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes,
+       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes + 1,
                             (void **)&cmdline_addr);
        if (status != EFI_SUCCESS)
                return NULL;

Note that the only other caller of efi_convert_cmdline() in x86-stub.c
ignores this value entirely.

Re: [PATCH 1/3] efi/libstub: fix efi_parse_options() ignoring the default command line

[Jonathan Marek]

On 10/12/24 3:46 AM, Ard Biesheuvel wrote:
> Hi Jonathan,
> 
> Please use a cover letter when sending more than a single patch.
> 
> On Sat, 12 Oct 2024 at 00:51, Jonathan Marek <jonathan@marek.ca> wrote:
>>
>> efi_convert_cmdline() always returns a size of at least 1 because it counts
>> the NUL terminator, so the "cmdline_size == 0" condition is not possible.
>>
>> Change it to compare against 1 to get the intended behavior: to use
>> CONFIG_CMDLINE when load_options_size is 0.
>>
>> Fixes: 60f38de7a8d4 ("efi/libstub: Unify command line param parsing")
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/firmware/efi/libstub/efi-stub.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
>> index 958a680e0660d..709ae2d41a632 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub.c
>> @@ -129,7 +129,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>>
>>          if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
>>              IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
>> -           cmdline_size == 0) {
>> +           cmdline_size == 1) {
> 
> I'd prefer it if we could keep the weirdness local to
> efi_convert_cmdline(). Would the below fix things too?
> 
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -395,9 +395,7 @@
>                  }
>          }
> 
> -       options_bytes++;        /* NUL termination */
> -
> -       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes,
> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes + 1,
>                               (void **)&cmdline_addr);
>          if (status != EFI_SUCCESS)
>                  return NULL;
> 
> Note that the only other caller of efi_convert_cmdline() in x86-stub.c
> ignores this value entirely.
> 

Just changing this would just make things more broken, the following 
snprintf would remove the last character of the command line because it 
uses options_bytes.

Since this patch has a Fixes: tag, I wanted to make the fix as simple as 
possible. If you think comparing the size to 1 is "weird", the fix could 
instead check if cmdline[0] is non-NUL (or just strlen(cmdline)==0 if 
you don't like that either).

And then my followup cleanup patch can just remove the cmd_line_len 
argument from efi_convert_cmdline().

Re: [PATCH 1/3] efi/libstub: fix efi_parse_options() ignoring the default command line

[Ard Biesheuvel]

On Sat, 12 Oct 2024 at 13:58, Jonathan Marek <jonathan@marek.ca> wrote:
>
> On 10/12/24 3:46 AM, Ard Biesheuvel wrote:
> > Hi Jonathan,
> >
> > Please use a cover letter when sending more than a single patch.
> >
> > On Sat, 12 Oct 2024 at 00:51, Jonathan Marek <jonathan@marek.ca> wrote:
> >>
> >> efi_convert_cmdline() always returns a size of at least 1 because it counts
> >> the NUL terminator, so the "cmdline_size == 0" condition is not possible.
> >>
> >> Change it to compare against 1 to get the intended behavior: to use
> >> CONFIG_CMDLINE when load_options_size is 0.
> >>
> >> Fixes: 60f38de7a8d4 ("efi/libstub: Unify command line param parsing")
> >> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> >> ---
> >>   drivers/firmware/efi/libstub/efi-stub.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> >> index 958a680e0660d..709ae2d41a632 100644
> >> --- a/drivers/firmware/efi/libstub/efi-stub.c
> >> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> >> @@ -129,7 +129,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
> >>
> >>          if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
> >>              IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
> >> -           cmdline_size == 0) {
> >> +           cmdline_size == 1) {
> >
> > I'd prefer it if we could keep the weirdness local to
> > efi_convert_cmdline(). Would the below fix things too?
> >
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -395,9 +395,7 @@
> >                  }
> >          }
> >
> > -       options_bytes++;        /* NUL termination */
> > -
> > -       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes,
> > +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes + 1,
> >                               (void **)&cmdline_addr);
> >          if (status != EFI_SUCCESS)
> >                  return NULL;
> >
> > Note that the only other caller of efi_convert_cmdline() in x86-stub.c
> > ignores this value entirely.
> >
>
> Just changing this would just make things more broken, the following
> snprintf would remove the last character of the command line because it
> uses options_bytes.
>

Ugh, you're right. So just use options_bytes - 1 in the assignment then.

> Since this patch has a Fixes: tag, I wanted to make the fix as simple as
> possible. If you think comparing the size to 1 is "weird", the fix could
> instead check if cmdline[0] is non-NUL (or just strlen(cmdline)==0 if
> you don't like that either).
>

Checking the value of the first byte is reasonable I think.

> And then my followup cleanup patch can just remove the cmd_line_len
> argument from efi_convert_cmdline().

Yes that would be fine - it is not really useful in any case.