Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llext: add support for init arrays #76724

Closed
wants to merge 2 commits into from
+90 −15
Closed

llext: add support for init arrays #76724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64f922a
llext: refactor section table handling
pillo79 Aug 5, 2024
5d15c03
llext: add support for init arrays
pillo79 Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/zephyr/llext/elf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ struct elf64_shdr {
#define SHT_NOBITS 0x8 /**< Program data with no file image */
#define SHT_REL 0x9 /**< Relocation entries without addends */
#define SHT_DYNSYM 0xB /**< Dynamic linking symbol table */
#define SHT_INIT_ARRAY 0xe /**< Array of pointers to init functions */
#define SHT_FINI_ARRAY 0xf /**< Array of pointers to termination functions */
#define SHT_PREINIT_ARRAY 0x10 /**< Array of pointers to early init functions */

/** ELF section flags */
#define SHF_WRITE 0x1 /**< Section is writable */
Expand Down
3 changes: 3 additions & 0 deletions include/zephyr/llext/llext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ enum llext_mem {
LLEXT_MEM_SYMTAB, /**< Symbol table */
LLEXT_MEM_STRTAB, /**< Symbol name strings */
LLEXT_MEM_SHSTRTAB, /**< Section name strings */
LLEXT_MEM_PREINIT, /**< Array of early init functions */
LLEXT_MEM_INIT, /**< Array of init functions */
LLEXT_MEM_FINI, /**< Array of termination functions */

LLEXT_MEM_COUNT, /**< Number of regions managed by LLEXT */
};
Expand Down
99 changes: 84 additions & 15 deletions subsys/llext/llext_load.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,15 @@ static int llext_load_elf_data(struct llext_loader *ldr, struct llext *ext)
*/
static int llext_find_tables(struct llext_loader *ldr)
{
int table_cnt, i;
int i;

memset(ldr->sects, 0, sizeof(ldr->sects));

/* Find symbol and string tables */
for (i = 0, table_cnt = 0; i < ldr->sect_cnt && table_cnt < 3; ++i) {
for (i = 0; i < ldr->sect_cnt; ++i) {
elf_shdr_t *shdr = ldr->sect_hdrs + i;
const char *sect_type_str;
enum llext_mem sect_mem_idx = LLEXT_MEM_COUNT;

LOG_DBG("section %d at 0x%zx: name %d, type %d, flags 0x%zx, "
"addr 0x%zx, size %zd, link %d, info %d",
Expand All @@ -162,32 +164,49 @@ static int llext_find_tables(struct llext_loader *ldr)
switch (shdr->sh_type) {
case SHT_SYMTAB:
case SHT_DYNSYM:
LOG_DBG("symtab at %d", i);
ldr->sects[LLEXT_MEM_SYMTAB] = *shdr;
ldr->sect_map[i].mem_idx = LLEXT_MEM_SYMTAB;
table_cnt++;
sect_type_str = "symtab";
sect_mem_idx = LLEXT_MEM_SYMTAB;
break;
case SHT_STRTAB:
if (ldr->hdr.e_shstrndx == i) {
LOG_DBG("shstrtab at %d", i);
ldr->sects[LLEXT_MEM_SHSTRTAB] = *shdr;
ldr->sect_map[i].mem_idx = LLEXT_MEM_SHSTRTAB;
sect_type_str = "shstrtab";
sect_mem_idx = LLEXT_MEM_SHSTRTAB;
} else {
LOG_DBG("strtab at %d", i);
ldr->sects[LLEXT_MEM_STRTAB] = *shdr;
ldr->sect_map[i].mem_idx = LLEXT_MEM_STRTAB;
sect_type_str = "strtab";
sect_mem_idx = LLEXT_MEM_STRTAB;
}
table_cnt++;
break;
default:
case SHT_PREINIT_ARRAY:
sect_type_str = "preinit";
sect_mem_idx = LLEXT_MEM_PREINIT;
break;
case SHT_INIT_ARRAY:
sect_type_str = "init";
sect_mem_idx = LLEXT_MEM_INIT;
break;
case SHT_FINI_ARRAY:
sect_type_str = "fini";
sect_mem_idx = LLEXT_MEM_FINI;
break;
default:
/* not a special section */
continue;
}

if (ldr->sects[sect_mem_idx].sh_type != 0) {
LOG_ERR("Multiple %s sections found", sect_type_str);
return -ENOEXEC;
}

LOG_DBG("%s at %d", sect_type_str, i);
ldr->sects[sect_mem_idx] = *shdr;
ldr->sect_map[i].mem_idx = sect_mem_idx;
}

if (!ldr->sects[LLEXT_MEM_SHSTRTAB].sh_type ||
!ldr->sects[LLEXT_MEM_STRTAB].sh_type ||
!ldr->sects[LLEXT_MEM_SYMTAB].sh_type) {
LOG_ERR("Some sections are missing or present multiple times!");
LOG_ERR("Some needed sections are missing");
return -ENOEXEC;
}

Expand All @@ -208,6 +227,12 @@ static int llext_map_sections(struct llext_loader *ldr, struct llext *ext)

name = llext_string(ldr, ext, LLEXT_MEM_SHSTRTAB, shdr->sh_name);

if (ldr->sect_map[i].mem_idx != LLEXT_MEM_COUNT) {
LOG_DBG("section %d name %s already mapped to region %d",
i, name, ldr->sect_map[i].mem_idx);
continue;
}

/* Identify the section type by its flags */
enum llext_mem mem_idx;

Expand Down Expand Up @@ -539,6 +564,43 @@ static int llext_copy_symbols(struct llext_loader *ldr, struct llext *ext,
return 0;
}

static int llext_call_array(struct llext_loader *ldr, struct llext *ext, enum llext_mem fn_array)
{
uintptr_t *ptr;
size_t nents, i;
void (*entry_fn)(void);

if (ldr->sects[fn_array].sh_size == 0)
return 0;

if (ldr->sects[fn_array].sh_entsize == 0 ||
ldr->sects[fn_array].sh_size % ldr->sects[fn_array].sh_entsize != 0) {
LOG_ERR("Invalid data in region %d", fn_array);
return -ENOEXEC;
}

nents = ldr->sects[fn_array].sh_size / ldr->sects[fn_array].sh_entsize;
ptr = ext->mem[fn_array];
for (i = 0; i < nents; ++i) {
entry_fn = (void *) *ptr++;
entry_fn();
}

return 0;
}

static int llext_call_inits(struct llext_loader *ldr, struct llext *ext)
{
int ret;

ret = llext_call_array(ldr, ext, LLEXT_MEM_PREINIT);
if (ret != 0) {
return ret;
}

return llext_call_array(ldr, ext, LLEXT_MEM_INIT);
}

/*
* Load a valid ELF as an extension
*/
Expand Down Expand Up @@ -630,6 +692,13 @@ int do_llext_load(struct llext_loader *ldr, struct llext *ext,
goto out;
}

LOG_DBG("Calling init functions...");
ret = llext_call_inits(ldr, ext);
Copy link
Collaborator

@teburd teburd Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changing the loading behavior from simple parse and copy of elf data to actually calling functions (executable code) in the elf file. Done outside of user mode even if a user wishes to sandbox an extension in a user mode thread.

We could do more validation/verification of symbolic linking. There's no way of validating these init functions aren't malicious.

I'm concerned this leaves a pretty large gap for untrusted code to inject malicious behavior and a user of llext may never know it.

Maybe its worth reviewing this behavior @ceolin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the elf is assumed to be untrusted by now (since there is no authenticity / integrity check). Execute these functions in the kernel context is definitely a problem. Can you defer this initialization for after the elf is loaded, and the user be responsible to call it assuming it is untrusted ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this is a valid point. I will make the llext_call_inits function part of the API so the user will need to explicitly call it if it's needed.

In the future however I would like to find a way to make this process automatic, like it is done for Linux modules. Definitely needs at least some kind of validation and an "llext header" describing Zephyr related features (such as should the initi function be user or kernel space), though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW SOF currently does sign its LLEXT objects and verifies signature correctness. If Zephyr decides to implement an API for this, SOF will probably also use it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh I think Zephyr should provide infrastructure to that, there are too many caveats and pretty much everyone using LLEXT in products will need it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pillo79 The Linux model is not exactly the same as here. Linux won't be blindly executing an userspace application in kernel context. If the module is trusted we may do how is proposed here, but we have to cover the untrusted case.
Maybe for untrusted modules we can sandbox the module before initializing it ?

if (ret != 0) {
LOG_ERR("Failed to init, ret %d", ret);
goto out;
}

out:
/*
* Free resources only used during loading. Note that this exploits
Expand Down
Loading