| Message ID | 20240513181757.84898-1-Maxim.Moskalets@kaspersky.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] cmd: move ELF load and boot to lib/elf.c | expand |
On 5/13/24 20:17, Maxim Moskalets wrote: > From: Maxim Moskalets <maximmosk4@gmail.com> > > Loading and running the ELF image is the responsibility of the > library and should not be associated with the command line interface. > > It is also required to run ELF images from FIT with the bootm command > so as not to depend on the command line interface. > > Signed-off-by: Maxim Moskalets <maximmosk4@gmail.com> > --- > cmd/elf.c | 49 +++++++++++++++++-------------------------------- > include/elf.h | 10 ++++++++++ > lib/elf.c | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 32 deletions(-) > > diff --git a/cmd/elf.c b/cmd/elf.c > index a02361f9f5..1fb955ae41 100644 > --- a/cmd/elf.c > +++ b/cmd/elf.c > @@ -19,21 +19,6 @@ > #include <linux/linkage.h> > #endif > > -/* Allow ports to override the default behavior */ > -static unsigned long do_bootelf_exec(ulong (*entry)(int, char * const[]), > - int argc, char *const argv[]) > -{ > - unsigned long ret; > - > - /* > - * pass address parameter as argv[0] (aka command name), > - * and all remaining args Carving out the library function is a good idea. Nits %s/args/arguments/ > - */ > - ret = entry(argc, argv); > - > - return ret; > -} > - > /* Interpreter command to boot an arbitrary ELF image from memory */ Please, document functions fully: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation The preferred place for documenting exported functions is in the header file. > int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > { > @@ -43,8 +28,8 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > #endif > unsigned long addr; /* Address of the ELF image */ > unsigned long rc; /* Return value from user code */ > - char *sload = NULL; > int rcode = 0; Maybe %s/0/CMD_RET_SUCCESS/ > + Bootelf_flags flags = {0}; > > /* Consume 'bootelf' */ > argc--; argv++; > @@ -52,7 +37,10 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > /* Check for [-p|-s] flag. */ > if (argc >= 1 && (argv[0][0] == '-' && \ > (argv[0][1] == 'p' || argv[0][1] == 's'))) { > - sload = argv[0]; > + if (argv[0][1] == 'p') > + flags.phdr = 1; > + printf("## Try to elf hdr format %s\n", > + flags.phdr ? "phdr" : "shdr"); Please, use log_debug() for debug messages and avoid the '## ' noise. Do you mean: log_debug("Using ELF header format %s\n", > /* Consume flag. */ > argc--; argv++; > } > @@ -75,14 +63,6 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > } else > addr = image_load_addr; > > - if (!valid_elf_image(addr)) > - return 1; > - > - if (sload && sload[1] == 'p') > - addr = load_elf_image_phdr(addr); > - else > - addr = load_elf_image_shdr(addr); > - > #if CONFIG_IS_ENABLED(CMD_ELF_FDT_SETUP) > if (fdt_addr) { > printf("## Setting up FDT at 0x%08lx ...\n", fdt_addr); This looks like a debug message too. > @@ -93,21 +73,26 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > } > #endif > > - if (!env_get_autostart()) > - return rcode; > - > - printf("## Starting application at 0x%08lx ...\n", addr); > - flush(); > + if (env_get_autostart()) { > + flags.autostart = 1; > + printf("## Starting application at 0x%08lx ...\n", addr); log_debug("Starting application at 0x%08lx ...\n", addr); > + flush(); > + } > > /* > * pass address parameter as argv[0] (aka command name), > * and all remaining args > */ > - rc = do_bootelf_exec((void *)addr, argc, argv); > + rc = bootelf(addr, flags, argc, argv); > if (rc != 0) > rcode = 1; %s/1/CMD_RET_FAILURE/ > > - printf("## Application terminated, rc = 0x%lx\n", rc); > + if (flags.autostart) { > + if (ENOEXEC == errno) if (rc && ENOEXEC == errno) > + printf("## Invalid ELF image\n"); log_err("Invalid ELF image\n"); > + else > + printf("## Application terminated, rc = 0x%lx\n", rc); This message is only relevant if the application failed: else if (rc) log_err("Application failed, rc = 0x%lx\n", rc); > + } > > return rcode; > } > diff --git a/include/elf.h b/include/elf.h > index a4ba74d8ab..b88e6cf403 100644 > --- a/include/elf.h > +++ b/include/elf.h > @@ -12,6 +12,12 @@ > #ifndef __ASSEMBLY__ > #include "compiler.h" > > +/* Flag param bits for bootelf() function */ > +typedef struct { > + unsigned phdr : 1; /* load via program (not section) headers */ > + unsigned autostart : 1; /* Start ELF after loading */ > +} Bootelf_flags; > + > /* This version doesn't work for 64-bit ABIs - Erik */ If the elf command does not work in certain ABIs we need to ensure via Kconfig that it is not build for these. > > /* These typedefs need to be handled better */ > @@ -700,6 +706,10 @@ unsigned long elf_hash(const unsigned char *name); > #define R_RISCV_RELATIVE 3 > > #ifndef __ASSEMBLY__ > +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), > + int argc, char *const argv[]); > +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, > + int argc, char *const argv[]); > int valid_elf_image(unsigned long addr); > unsigned long load_elf64_image_phdr(unsigned long addr); > unsigned long load_elf64_image_shdr(unsigned long addr); > diff --git a/lib/elf.c b/lib/elf.c > index 9a794f9cba..f8eeceef3d 100644 > --- a/lib/elf.c > +++ b/lib/elf.c > @@ -7,6 +7,7 @@ > #include <cpu_func.h> > #include <elf.h> > #include <env.h> > +#include <errno.h> > #include <net.h> > #include <vxworks.h> > #ifdef CONFIG_X86 > @@ -15,6 +16,43 @@ > #include <linux/linkage.h> > #endif > > +/* Allow ports to override the default behavior */ Please, fully document the function. > +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), > + int argc, char *const argv[]) > +{ > + return entry(argc, argv); > +} > + > +/* > + * @brief Boot ELF from memory > + * > + * @param addr Loading address of ELF in memory > + * @param flags Bits like ELF_PHDR to control boot details > + * @param argc, argv May be used to pass command line arguments (maybe unused) > + * Necessary for backward compatibility with the CLI command > + * @retuen Number returned by ELF application This does not match our documentation style, see https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > + * > + * Sets errno = ENOEXEC if ELF image is not valid %s/if ELF/if the ELF/ > + */ > +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, > + int argc, char *const argv[]) > +{ > + unsigned long entry_addr; > + > + if (!valid_elf_image(addr)) { > + errno = ENOEXEC; > + return 1; > + } > + > + entry_addr = flags.phdr ? load_elf_image_phdr(addr) > + : load_elf_image_shdr(addr); > + > + if (!flags.autostart) > + return 0; Please, return early. We don't need entry_addr here. > + > + return bootelf_exec((void *)entry_addr, argc, argv);; Duplicate ';'. An ELF binary may expect that argc >= 1 and argv[0] being the binary name. How do we ensure this? We should describe in a man-page doc/usage/cmd/elf.rst that the first extraneous argument passed to the elf command is argv[0]? On some paths we have not set errno which means it has a random value set in unrelated functions. How would we able to evaluate errno in the caller if the return value is non-zero? Please, initialize errno to 0 at the start of the function. We need a test case. E.g. we could build an ELF binary returning the CRC32 of the concatenated arguments. Best regards Heinrich > +} > + > /* > * A very simple ELF64 loader, assumes the image is valid, returns the > * entry point address.
On 18.05.2024 12:40, Heinrich Schuchardt wrote: > On 5/13/24 20:17, Maxim Moskalets wrote: >> From: Maxim Moskalets <maximmosk4@gmail.com> >> >> Loading and running the ELF image is the responsibility of the >> library and should not be associated with the command line interface. >> >> It is also required to run ELF images from FIT with the bootm command >> so as not to depend on the command line interface. >> >> Signed-off-by: Maxim Moskalets <maximmosk4@gmail.com> >> --- >> cmd/elf.c | 49 +++++++++++++++++-------------------------------- >> include/elf.h | 10 ++++++++++ >> lib/elf.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 65 insertions(+), 32 deletions(-) >> >> diff --git a/cmd/elf.c b/cmd/elf.c >> index a02361f9f5..1fb955ae41 100644 >> --- a/cmd/elf.c >> +++ b/cmd/elf.c >> @@ -19,21 +19,6 @@ >> #include <linux/linkage.h> >> #endif >> >> -/* Allow ports to override the default behavior */ >> -static unsigned long do_bootelf_exec(ulong (*entry)(int, char * >> const[]), >> - int argc, char *const argv[]) >> -{ >> - unsigned long ret; >> - >> - /* >> - * pass address parameter as argv[0] (aka command name), >> - * and all remaining args > > Carving out the library function is a good idea. > > Nits > > %s/args/arguments/ > >> - */ >> - ret = entry(argc, argv); >> - >> - return ret; >> -} >> - >> /* Interpreter command to boot an arbitrary ELF image from memory */ > > Please, document functions fully: > > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > > > The preferred place for documenting exported functions is in the header > file. > Not my code, but okay. >> int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char >> *const argv[]) >> { >> @@ -43,8 +28,8 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int >> argc, char *const argv[]) >> #endif >> unsigned long addr; /* Address of the ELF image */ >> unsigned long rc; /* Return value from user code */ >> - char *sload = NULL; >> int rcode = 0; > > Maybe > > %s/0/CMD_RET_SUCCESS/ > >> + Bootelf_flags flags = {0}; >> >> /* Consume 'bootelf' */ >> argc--; argv++; >> @@ -52,7 +37,10 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, >> int argc, char *const argv[]) >> /* Check for [-p|-s] flag. */ >> if (argc >= 1 && (argv[0][0] == '-' && \ >> (argv[0][1] == 'p' || argv[0][1] == 's'))) { >> - sload = argv[0]; >> + if (argv[0][1] == 'p') >> + flags.phdr = 1; >> + printf("## Try to elf hdr format %s\n", >> + flags.phdr ? "phdr" : "shdr"); > > Please, use log_debug() for debug messages and avoid the '## ' noise. > > Do you mean: > > log_debug("Using ELF header format %s\n", > It was in original implementation, but i can fix. >> /* Consume flag. */ >> argc--; argv++; >> } >> @@ -75,14 +63,6 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, >> int argc, char *const argv[]) >> } else >> addr = image_load_addr; >> >> - if (!valid_elf_image(addr)) >> - return 1; >> - >> - if (sload && sload[1] == 'p') >> - addr = load_elf_image_phdr(addr); >> - else >> - addr = load_elf_image_shdr(addr); >> - >> #if CONFIG_IS_ENABLED(CMD_ELF_FDT_SETUP) >> if (fdt_addr) { >> printf("## Setting up FDT at 0x%08lx ...\n", fdt_addr); > > This looks like a debug message too. > >> @@ -93,21 +73,26 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, >> int argc, char *const argv[]) >> } >> #endif >> >> - if (!env_get_autostart()) >> - return rcode; >> - >> - printf("## Starting application at 0x%08lx ...\n", addr); >> - flush(); >> + if (env_get_autostart()) { >> + flags.autostart = 1; >> + printf("## Starting application at 0x%08lx ...\n", addr); > > log_debug("Starting application at 0x%08lx ...\n", addr); > >> + flush(); >> + } >> >> /* >> * pass address parameter as argv[0] (aka command name), >> * and all remaining args >> */ >> - rc = do_bootelf_exec((void *)addr, argc, argv); >> + rc = bootelf(addr, flags, argc, argv); >> if (rc != 0) >> rcode = 1; > > %s/1/CMD_RET_FAILURE/ > >> >> - printf("## Application terminated, rc = 0x%lx\n", rc); >> + if (flags.autostart) { >> + if (ENOEXEC == errno) > > if (rc && ENOEXEC == errno) > >> + printf("## Invalid ELF image\n"); > > log_err("Invalid ELF image\n"); > >> + else >> + printf("## Application terminated, rc = 0x%lx\n", rc); > > This message is only relevant if the application failed: > > else if (rc) > log_err("Application failed, rc = 0x%lx\n", rc); > >> + } >> >> return rcode; >> } >> diff --git a/include/elf.h b/include/elf.h >> index a4ba74d8ab..b88e6cf403 100644 >> --- a/include/elf.h >> +++ b/include/elf.h >> @@ -12,6 +12,12 @@ >> #ifndef __ASSEMBLY__ >> #include "compiler.h" >> >> +/* Flag param bits for bootelf() function */ >> +typedef struct { >> + unsigned phdr : 1; /* load via program (not section) >> headers */ >> + unsigned autostart : 1; /* Start ELF after loading */ >> +} Bootelf_flags; >> + >> /* This version doesn't work for 64-bit ABIs - Erik */ > > If the elf command does not work in certain ABIs we need to ensure via > Kconfig that it is not build for these. > Not my comment, i don't know. >> >> /* These typedefs need to be handled better */ >> @@ -700,6 +706,10 @@ unsigned long elf_hash(const unsigned char *name); >> #define R_RISCV_RELATIVE 3 >> >> #ifndef __ASSEMBLY__ >> +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), >> + int argc, char *const argv[]); >> +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, >> + int argc, char *const argv[]); >> int valid_elf_image(unsigned long addr); >> unsigned long load_elf64_image_phdr(unsigned long addr); >> unsigned long load_elf64_image_shdr(unsigned long addr); >> diff --git a/lib/elf.c b/lib/elf.c >> index 9a794f9cba..f8eeceef3d 100644 >> --- a/lib/elf.c >> +++ b/lib/elf.c >> @@ -7,6 +7,7 @@ >> #include <cpu_func.h> >> #include <elf.h> >> #include <env.h> >> +#include <errno.h> >> #include <net.h> >> #include <vxworks.h> >> #ifdef CONFIG_X86 >> @@ -15,6 +16,43 @@ >> #include <linux/linkage.h> >> #endif >> >> +/* Allow ports to override the default behavior */ > > Please, fully document the function. > >> +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), >> + int argc, char *const argv[]) >> +{ >> + return entry(argc, argv); >> +} >> + >> +/* >> + * @brief Boot ELF from memory >> + * >> + * @param addr Loading address of ELF in memory >> + * @param flags Bits like ELF_PHDR to control boot details >> + * @param argc, argv May be used to pass command line arguments >> (maybe unused) >> + * Necessary for backward compatibility with the CLI >> command >> + * @retuen Number returned by ELF application > > This does not match our documentation style, see > > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > > >> + * >> + * Sets errno = ENOEXEC if ELF image is not valid > > %s/if ELF/if the ELF/ > >> + */ >> +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, >> + int argc, char *const argv[]) >> +{ >> + unsigned long entry_addr; >> + >> + if (!valid_elf_image(addr)) { >> + errno = ENOEXEC; >> + return 1; >> + } >> + >> + entry_addr = flags.phdr ? load_elf_image_phdr(addr) >> + : load_elf_image_shdr(addr); >> + >> + if (!flags.autostart) >> + return 0; > > Please, return early. We don't need entry_addr here. > >> + >> + return bootelf_exec((void *)entry_addr, argc, argv);; > > Duplicate ';'. > > An ELF binary may expect that argc >= 1 and argv[0] being the binary > name. How do we ensure this? > Can i use dummy argc = 1 and argv = {"", NULL}; if (!argc && !argv)? > We should describe in a man-page doc/usage/cmd/elf.rst that the first > extraneous argument passed to the elf command is argv[0]? > It is impossible for bootelf CLI command. In FIT we can describe behavior (in next patch with ELFs in FIT). > On some paths we have not set errno which means it has a random value > set in unrelated functions. How would we able to evaluate errno in the > caller if the return value is non-zero? > > Please, initialize errno to 0 at the start of the function. > > We need a test case. E.g. we could build an ELF binary returning the > CRC32 of the concatenated arguments. > Not in this patch, but okay. What about example in sandbox tests or/and example app? Best regards Maxim > Best regards > > Heinrich > >> +} >> + >> /* >> * A very simple ELF64 loader, assumes the image is valid, returns the >> * entry point address. >
diff --git a/cmd/elf.c b/cmd/elf.c index a02361f9f5..1fb955ae41 100644 --- a/cmd/elf.c +++ b/cmd/elf.c @@ -19,21 +19,6 @@ #include <linux/linkage.h> #endif -/* Allow ports to override the default behavior */ -static unsigned long do_bootelf_exec(ulong (*entry)(int, char * const[]), - int argc, char *const argv[]) -{ - unsigned long ret; - - /* - * pass address parameter as argv[0] (aka command name), - * and all remaining args - */ - ret = entry(argc, argv); - - return ret; -} - /* Interpreter command to boot an arbitrary ELF image from memory */ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { @@ -43,8 +28,8 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) #endif unsigned long addr; /* Address of the ELF image */ unsigned long rc; /* Return value from user code */ - char *sload = NULL; int rcode = 0; + Bootelf_flags flags = {0}; /* Consume 'bootelf' */ argc--; argv++; @@ -52,7 +37,10 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) /* Check for [-p|-s] flag. */ if (argc >= 1 && (argv[0][0] == '-' && \ (argv[0][1] == 'p' || argv[0][1] == 's'))) { - sload = argv[0]; + if (argv[0][1] == 'p') + flags.phdr = 1; + printf("## Try to elf hdr format %s\n", + flags.phdr ? "phdr" : "shdr"); /* Consume flag. */ argc--; argv++; } @@ -75,14 +63,6 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } else addr = image_load_addr; - if (!valid_elf_image(addr)) - return 1; - - if (sload && sload[1] == 'p') - addr = load_elf_image_phdr(addr); - else - addr = load_elf_image_shdr(addr); - #if CONFIG_IS_ENABLED(CMD_ELF_FDT_SETUP) if (fdt_addr) { printf("## Setting up FDT at 0x%08lx ...\n", fdt_addr); @@ -93,21 +73,26 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } #endif - if (!env_get_autostart()) - return rcode; - - printf("## Starting application at 0x%08lx ...\n", addr); - flush(); + if (env_get_autostart()) { + flags.autostart = 1; + printf("## Starting application at 0x%08lx ...\n", addr); + flush(); + } /* * pass address parameter as argv[0] (aka command name), * and all remaining args */ - rc = do_bootelf_exec((void *)addr, argc, argv); + rc = bootelf(addr, flags, argc, argv); if (rc != 0) rcode = 1; - printf("## Application terminated, rc = 0x%lx\n", rc); + if (flags.autostart) { + if (ENOEXEC == errno) + printf("## Invalid ELF image\n"); + else + printf("## Application terminated, rc = 0x%lx\n", rc); + } return rcode; } diff --git a/include/elf.h b/include/elf.h index a4ba74d8ab..b88e6cf403 100644 --- a/include/elf.h +++ b/include/elf.h @@ -12,6 +12,12 @@ #ifndef __ASSEMBLY__ #include "compiler.h" +/* Flag param bits for bootelf() function */ +typedef struct { + unsigned phdr : 1; /* load via program (not section) headers */ + unsigned autostart : 1; /* Start ELF after loading */ +} Bootelf_flags; + /* This version doesn't work for 64-bit ABIs - Erik */ /* These typedefs need to be handled better */ @@ -700,6 +706,10 @@ unsigned long elf_hash(const unsigned char *name); #define R_RISCV_RELATIVE 3 #ifndef __ASSEMBLY__ +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), + int argc, char *const argv[]); +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, + int argc, char *const argv[]); int valid_elf_image(unsigned long addr); unsigned long load_elf64_image_phdr(unsigned long addr); unsigned long load_elf64_image_shdr(unsigned long addr); diff --git a/lib/elf.c b/lib/elf.c index 9a794f9cba..f8eeceef3d 100644 --- a/lib/elf.c +++ b/lib/elf.c @@ -7,6 +7,7 @@ #include <cpu_func.h> #include <elf.h> #include <env.h> +#include <errno.h> #include <net.h> #include <vxworks.h> #ifdef CONFIG_X86 @@ -15,6 +16,43 @@ #include <linux/linkage.h> #endif +/* Allow ports to override the default behavior */ +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]), + int argc, char *const argv[]) +{ + return entry(argc, argv); +} + +/* + * @brief Boot ELF from memory + * + * @param addr Loading address of ELF in memory + * @param flags Bits like ELF_PHDR to control boot details + * @param argc, argv May be used to pass command line arguments (maybe unused) + * Necessary for backward compatibility with the CLI command + * @retuen Number returned by ELF application + * + * Sets errno = ENOEXEC if ELF image is not valid + */ +unsigned long bootelf(unsigned long addr, Bootelf_flags flags, + int argc, char *const argv[]) +{ + unsigned long entry_addr; + + if (!valid_elf_image(addr)) { + errno = ENOEXEC; + return 1; + } + + entry_addr = flags.phdr ? load_elf_image_phdr(addr) + : load_elf_image_shdr(addr); + + if (!flags.autostart) + return 0; + + return bootelf_exec((void *)entry_addr, argc, argv);; +} + /* * A very simple ELF64 loader, assumes the image is valid, returns the * entry point address.