| Message ID | 20241011224812.25763-1-jonathan@marek.ca |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] efi/libstub: fix efi_parse_options() ignoring the default command line | expand |
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.
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().
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.
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");
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(-)