| Message ID | 20231110013817.2378507-6-danielwa@cisco.com |
|---|---|
| State | New |
| Headers | show |
| Series | generic command line v6 | expand |
Le 10/11/2023 à 02:38, Daniel Walker a écrit : > This adds code to handle the generic command line changes. > The efi code appears that it doesn't benefit as much from this design > as it could. So what can we do to improve that ? > > For example, if you had a prepend command line with "nokaslr" then > you might be helpful to re-enable it in the boot loader or dts, s/you/it > but there appears to be no way to re-enable kaslr or some of the > other options. > > The efi command line handling is incorrect. x86 and arm have an append > system however the efi code prepends the command line. > > For example, you could have a non-upgradable bios which sends > > efi=disable_early_pci_dma > > This hypothetically could have been set because early pci dma caused > issues on early versions of the product. > > Then later the early pci dma was made to work and the company desired > to start using it. To override the bios you could set the CONFIG_CMDLINE > to, > > efi=no_disable_early_pci_dma > > then parsing would normally start with the bios command line, then move > to the CONFIG_CMDLINE and you would end up with early pci dma turned on. > > however, current efi code keeps early pci dma off because the bios > arguments always override the built in. > > Per my reading this is different from the main body of x86, arm, and > arm64. > > The generic command line provides both append and prepend, so it > alleviates this issue if it's used. However not all architectures use > it. > > It would be desirable to allow the efi stub to have it's builtin command > line to be modified after compile, but I don't see a feasible way to do > that currently. Would be desirable or is desirable ? Your explanations are unclear. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker <danielwa@cisco.com> > --- > .../firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++++++++++ > drivers/firmware/efi/libstub/efi-stub.c | 9 ++++++ > drivers/firmware/efi/libstub/efistub.h | 1 + > drivers/firmware/efi/libstub/x86-stub.c | 14 +++++++-- > 4 files changed, 51 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index bfa30625f5d0..952fa2cdff51 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -11,6 +11,7 @@ > > #include <linux/efi.h> > #include <linux/kernel.h> > +#include <linux/cmdline.h> > #include <asm/efi.h> > #include <asm/setup.h> > > @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void) > return !efi_nosoftreserve; > } > > +/** > + * efi_handle_cmdline() - handle adding in built-in parts of the command line > + * @cmdline: kernel command line > + * > + * Add in the generic parts of the commandline and start the parsing of the > + * command line. > + * > + * Return: status code > + */ > +efi_status_t efi_handle_builtin_cmdline(char const *cmdline) > +{ > + efi_status_t status = EFI_SUCCESS; > + > + if (sizeof(CMDLINE_STATIC_PREPEND) > 1) > + status |= efi_parse_options(CMDLINE_STATIC_PREPEND); > + > + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) > + status |= efi_parse_options(cmdline); > + > + if (sizeof(CMDLINE_STATIC_APPEND) > 1) > + status |= efi_parse_options(CMDLINE_STATIC_APPEND); > + > + if (status != EFI_SUCCESS) > + efi_err("Failed to parse options\n"); > + > + return status; > +} > + > /** > * efi_parse_options() - Parse EFI command line options > * @cmdline: kernel command line > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index f9c1e8a2bd1d..770abe95c0ee 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) > return EFI_OUT_OF_RESOURCES; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_builtin_cmdline(cmdline); > + if (status != EFI_SUCCESS) { > + goto fail_free_cmdline; > + } > +#endif > + > +#ifdef CONFIG_CMDLINE > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > IS_ENABLED(CONFIG_CMDLINE_FORCE) || > cmdline_size == 0) { > @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) > goto fail_free_cmdline; > } > } > +#endif > > *cmdline_ptr = cmdline; > return EFI_SUCCESS; > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index 212687c30d79..1ac6631905c5 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, > unsigned long alignment, > unsigned long min_addr); > > +efi_status_t efi_handle_builtin_cmdline(char const *cmdline); > efi_status_t efi_parse_options(char const *cmdline); > > void efi_parse_option_graphics(char *option); > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index 9d5df683f882..273a8a9c8bbb 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > struct setup_header *hdr = &boot_params->hdr; > const struct linux_efi_initrd *initrd = NULL; > unsigned long kernel_entry; > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > + ((u64)boot_params->ext_cmd_line_ptr << 32)); > efi_status_t status; > > boot_params_pointer = boot_params; > @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > goto fail; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_builtin_cmdline((char *)cmdline_paddr); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + goto fail; > + } > +#else /* CONFIG_GENERIC_CMDLINE */ > + > #ifdef CONFIG_CMDLINE_BOOL > status = efi_parse_options(CONFIG_CMDLINE); > if (status != EFI_SUCCESS) { > @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > } > #endif > if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > - ((u64)boot_params->ext_cmd_line_ptr << 32)); > status = efi_parse_options((char *)cmdline_paddr); > if (status != EFI_SUCCESS) { > efi_err("Failed to parse options\n"); > @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > } > } > > +#endif > + > status = efi_decompress_kernel(&kernel_entry); > if (status != EFI_SUCCESS) { > efi_err("Failed to decompress kernel\n");
On Fri, 10 Nov 2023 at 02:39, Daniel Walker <danielwa@cisco.com> wrote: > > This adds code to handle the generic command line changes. > The efi code appears that it doesn't benefit as much from this design > as it could. > > For example, if you had a prepend command line with "nokaslr" then > you might be helpful to re-enable it in the boot loader or dts, > but there appears to be no way to re-enable kaslr or some of the > other options. > > The efi command line handling is incorrect. x86 and arm have an append > system however the efi code prepends the command line. > > For example, you could have a non-upgradable bios which sends > > efi=disable_early_pci_dma > > This hypothetically could have been set because early pci dma caused > issues on early versions of the product. > > Then later the early pci dma was made to work and the company desired > to start using it. To override the bios you could set the CONFIG_CMDLINE > to, > > efi=no_disable_early_pci_dma > > then parsing would normally start with the bios command line, then move > to the CONFIG_CMDLINE and you would end up with early pci dma turned on. > > however, current efi code keeps early pci dma off because the bios > arguments always override the built in. > > Per my reading this is different from the main body of x86, arm, and > arm64. > > The generic command line provides both append and prepend, so it > alleviates this issue if it's used. However not all architectures use > it. > > It would be desirable to allow the efi stub to have it's builtin command > line to be modified after compile, but I don't see a feasible way to do > that currently. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker <danielwa@cisco.com> There are quite some 'might be's and 'hypothetical's in this commit log. Is there an actual use case that you are addressing here? Without that, this looks like unnecessary churn to me, tbh. Note that this code executes in the context of platform firmware, which may be old and buggy so we should be cautious about making unnecessary changes here. > --- > .../firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++++++++++ > drivers/firmware/efi/libstub/efi-stub.c | 9 ++++++ > drivers/firmware/efi/libstub/efistub.h | 1 + > drivers/firmware/efi/libstub/x86-stub.c | 14 +++++++-- > 4 files changed, 51 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index bfa30625f5d0..952fa2cdff51 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -11,6 +11,7 @@ > > #include <linux/efi.h> > #include <linux/kernel.h> > +#include <linux/cmdline.h> > #include <asm/efi.h> > #include <asm/setup.h> > > @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void) > return !efi_nosoftreserve; > } > > +/** > + * efi_handle_cmdline() - handle adding in built-in parts of the command line > + * @cmdline: kernel command line > + * > + * Add in the generic parts of the commandline and start the parsing of the > + * command line. > + * > + * Return: status code > + */ > +efi_status_t efi_handle_builtin_cmdline(char const *cmdline) > +{ > + efi_status_t status = EFI_SUCCESS; > + > + if (sizeof(CMDLINE_STATIC_PREPEND) > 1) > + status |= efi_parse_options(CMDLINE_STATIC_PREPEND); > + > + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) > + status |= efi_parse_options(cmdline); > + > + if (sizeof(CMDLINE_STATIC_APPEND) > 1) > + status |= efi_parse_options(CMDLINE_STATIC_APPEND); > + > + if (status != EFI_SUCCESS) > + efi_err("Failed to parse options\n"); > + > + return status; > +} > + > /** > * efi_parse_options() - Parse EFI command line options > * @cmdline: kernel command line > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index f9c1e8a2bd1d..770abe95c0ee 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) > return EFI_OUT_OF_RESOURCES; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_builtin_cmdline(cmdline); > + if (status != EFI_SUCCESS) { > + goto fail_free_cmdline; > + } > +#endif > + > +#ifdef CONFIG_CMDLINE > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > IS_ENABLED(CONFIG_CMDLINE_FORCE) || > cmdline_size == 0) { > @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) > goto fail_free_cmdline; > } > } > +#endif > > *cmdline_ptr = cmdline; > return EFI_SUCCESS; > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index 212687c30d79..1ac6631905c5 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, > unsigned long alignment, > unsigned long min_addr); > > +efi_status_t efi_handle_builtin_cmdline(char const *cmdline); > efi_status_t efi_parse_options(char const *cmdline); > > void efi_parse_option_graphics(char *option); > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index 9d5df683f882..273a8a9c8bbb 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > struct setup_header *hdr = &boot_params->hdr; > const struct linux_efi_initrd *initrd = NULL; > unsigned long kernel_entry; > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > + ((u64)boot_params->ext_cmd_line_ptr << 32)); > efi_status_t status; > > boot_params_pointer = boot_params; > @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > goto fail; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_builtin_cmdline((char *)cmdline_paddr); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + goto fail; > + } > +#else /* CONFIG_GENERIC_CMDLINE */ > + > #ifdef CONFIG_CMDLINE_BOOL > status = efi_parse_options(CONFIG_CMDLINE); > if (status != EFI_SUCCESS) { > @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > } > #endif > if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > - ((u64)boot_params->ext_cmd_line_ptr << 32)); > status = efi_parse_options((char *)cmdline_paddr); > if (status != EFI_SUCCESS) { > efi_err("Failed to parse options\n"); > @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, > } > } > > +#endif > + > status = efi_decompress_kernel(&kernel_entry); > if (status != EFI_SUCCESS) { > efi_err("Failed to decompress kernel\n"); > -- > 2.39.2 > >
On Tue, Dec 12, 2023 at 10:55:33AM +0100, Ard Biesheuvel wrote: > On Fri, 10 Nov 2023 at 02:39, Daniel Walker <danielwa@cisco.com> wrote: > > > > This adds code to handle the generic command line changes. > > The efi code appears that it doesn't benefit as much from this design > > as it could. > > > > For example, if you had a prepend command line with "nokaslr" then > > you might be helpful to re-enable it in the boot loader or dts, > > but there appears to be no way to re-enable kaslr or some of the > > other options. > > > > The efi command line handling is incorrect. x86 and arm have an append > > system however the efi code prepends the command line. > > > > For example, you could have a non-upgradable bios which sends > > > > efi=disable_early_pci_dma > > > > This hypothetically could have been set because early pci dma caused > > issues on early versions of the product. > > > > Then later the early pci dma was made to work and the company desired > > to start using it. To override the bios you could set the CONFIG_CMDLINE > > to, > > > > efi=no_disable_early_pci_dma > > > > then parsing would normally start with the bios command line, then move > > to the CONFIG_CMDLINE and you would end up with early pci dma turned on. > > > > however, current efi code keeps early pci dma off because the bios > > arguments always override the built in. > > > > Per my reading this is different from the main body of x86, arm, and > > arm64. > > > > The generic command line provides both append and prepend, so it > > alleviates this issue if it's used. However not all architectures use > > it. > > > > It would be desirable to allow the efi stub to have it's builtin command > > line to be modified after compile, but I don't see a feasible way to do > > that currently. > > > > Cc: xe-linux-external@cisco.com > > Signed-off-by: Daniel Walker <danielwa@cisco.com> > > There are quite some 'might be's and 'hypothetical's in this commit log. > > Is there an actual use case that you are addressing here? Without > that, this looks like unnecessary churn to me, tbh. > > Note that this code executes in the context of platform firmware, > which may be old and buggy so we should be cautious about making > unnecessary changes here. It's been a while since I wrote this patch description, but there is an example I provided in the description. If you intend to enable early pci dma the command line parsing may still disable it because of how the parsing works inside the efi stub. There is an implementation difference between the efi stub and the architectures which results in this problem. This was not an issue I observed in the wild, it was the results of code review of the efi stub. Maybe the greater issue is that the efi stub seems to have connected itself to the kernels command line system. If the kernel changes then EFI stub must also change. Daniel
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index bfa30625f5d0..952fa2cdff51 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -11,6 +11,7 @@ #include <linux/efi.h> #include <linux/kernel.h> +#include <linux/cmdline.h> #include <asm/efi.h> #include <asm/setup.h> @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void) return !efi_nosoftreserve; } +/** + * efi_handle_cmdline() - handle adding in built-in parts of the command line + * @cmdline: kernel command line + * + * Add in the generic parts of the commandline and start the parsing of the + * command line. + * + * Return: status code + */ +efi_status_t efi_handle_builtin_cmdline(char const *cmdline) +{ + efi_status_t status = EFI_SUCCESS; + + if (sizeof(CMDLINE_STATIC_PREPEND) > 1) + status |= efi_parse_options(CMDLINE_STATIC_PREPEND); + + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) + status |= efi_parse_options(cmdline); + + if (sizeof(CMDLINE_STATIC_APPEND) > 1) + status |= efi_parse_options(CMDLINE_STATIC_APPEND); + + if (status != EFI_SUCCESS) + efi_err("Failed to parse options\n"); + + return status; +} + /** * efi_parse_options() - Parse EFI command line options * @cmdline: kernel command line diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c index f9c1e8a2bd1d..770abe95c0ee 100644 --- a/drivers/firmware/efi/libstub/efi-stub.c +++ b/drivers/firmware/efi/libstub/efi-stub.c @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) return EFI_OUT_OF_RESOURCES; } +#ifdef CONFIG_GENERIC_CMDLINE + status = efi_handle_builtin_cmdline(cmdline); + if (status != EFI_SUCCESS) { + goto fail_free_cmdline; + } +#endif + +#ifdef CONFIG_CMDLINE if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || IS_ENABLED(CONFIG_CMDLINE_FORCE) || cmdline_size == 0) { @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) goto fail_free_cmdline; } } +#endif *cmdline_ptr = cmdline; return EFI_SUCCESS; diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 212687c30d79..1ac6631905c5 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, unsigned long alignment, unsigned long min_addr); +efi_status_t efi_handle_builtin_cmdline(char const *cmdline); efi_status_t efi_parse_options(char const *cmdline); void efi_parse_option_graphics(char *option); diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index 9d5df683f882..273a8a9c8bbb 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, struct setup_header *hdr = &boot_params->hdr; const struct linux_efi_initrd *initrd = NULL; unsigned long kernel_entry; + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | + ((u64)boot_params->ext_cmd_line_ptr << 32)); efi_status_t status; boot_params_pointer = boot_params; @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle, goto fail; } +#ifdef CONFIG_GENERIC_CMDLINE + status = efi_handle_builtin_cmdline((char *)cmdline_paddr); + if (status != EFI_SUCCESS) { + efi_err("Failed to parse options\n"); + goto fail; + } +#else /* CONFIG_GENERIC_CMDLINE */ + #ifdef CONFIG_CMDLINE_BOOL status = efi_parse_options(CONFIG_CMDLINE); if (status != EFI_SUCCESS) { @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle, } #endif if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | - ((u64)boot_params->ext_cmd_line_ptr << 32)); status = efi_parse_options((char *)cmdline_paddr); if (status != EFI_SUCCESS) { efi_err("Failed to parse options\n"); @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle, } } +#endif + status = efi_decompress_kernel(&kernel_entry); if (status != EFI_SUCCESS) { efi_err("Failed to decompress kernel\n");
This adds code to handle the generic command line changes. The efi code appears that it doesn't benefit as much from this design as it could. For example, if you had a prepend command line with "nokaslr" then you might be helpful to re-enable it in the boot loader or dts, but there appears to be no way to re-enable kaslr or some of the other options. The efi command line handling is incorrect. x86 and arm have an append system however the efi code prepends the command line. For example, you could have a non-upgradable bios which sends efi=disable_early_pci_dma This hypothetically could have been set because early pci dma caused issues on early versions of the product. Then later the early pci dma was made to work and the company desired to start using it. To override the bios you could set the CONFIG_CMDLINE to, efi=no_disable_early_pci_dma then parsing would normally start with the bios command line, then move to the CONFIG_CMDLINE and you would end up with early pci dma turned on. however, current efi code keeps early pci dma off because the bios arguments always override the built in. Per my reading this is different from the main body of x86, arm, and arm64. The generic command line provides both append and prepend, so it alleviates this issue if it's used. However not all architectures use it. It would be desirable to allow the efi stub to have it's builtin command line to be modified after compile, but I don't see a feasible way to do that currently. Cc: xe-linux-external@cisco.com Signed-off-by: Daniel Walker <danielwa@cisco.com> --- .../firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++++++++++ drivers/firmware/efi/libstub/efi-stub.c | 9 ++++++ drivers/firmware/efi/libstub/efistub.h | 1 + drivers/firmware/efi/libstub/x86-stub.c | 14 +++++++-- 4 files changed, 51 insertions(+), 2 deletions(-)