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