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.

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

Conversation

pillo79
Copy link
Collaborator

@pillo79 pillo79 commented Aug 6, 2024

Add support for the .preinit_array and .init_array sections in ELF files. These sections are arrays of function pointers that are filled by the compiler with the addresses of functions, such as C++ constructors, that need to be called at startup by the loader.

Supporting the .fini_array section requires more changes and is not currently implemented.

A test for this feature and several other C++-related changes will be forthcoming.

This patch refactors the section table handling in the ELF loader to
make it generic and prepare it for optional special sections.

Signed-off-by: Luca Burelli <[email protected]>
Add support for the .preinit_array and .init_array sections in ELF
files. These sections are arrays of function pointers that are filled by
the compiler with the addresses of functions that need to be called at
startup by the loader, such as C++ constructors.

Supporting the .fini_array section requires more changes and is not
currently implemented.

Signed-off-by: Luca Burelli <[email protected]>
@pillo79 pillo79 marked this pull request as ready for review August 6, 2024 15:47
@zephyrbot zephyrbot added the area: llext Linkable Loadable Extensions label Aug 6, 2024
@zephyrbot zephyrbot requested a review from lyakh August 6, 2024 15:47
@@ -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

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 ?

Copy link
Collaborator

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

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

Besides the already discussed issue , LGTM.

As for the 'executing arbitrary code in kernel mode as part of loading' issue, one way forward I can see is to have the application perform the calls manually (using llext_find_section - however, new API might be needed, c.f. #74568 (comment)).
A reference implementation could be provided in LLEXT headers so that a single e.g. llext_execute_init_functions call is done from user application (would have to be in-header static inline - can't be a syscall since AFAIK, they always run in kernel mode, which we don't want here).

A second solution I see is adding a perform_init or w/e flag in struct llext_load_param, which defaults to false. One issue with this is how to distinguish between kernel and user extensions; it would also require the syscall to drop privileges and execute a tiny snippet of user code as part of its execution, which I'm not sure is possible in Zephyr.

@pillo79
Copy link
Collaborator Author

pillo79 commented Aug 28, 2024

Thanks for the feedback! Closing this in favor of #77641.

@pillo79 pillo79 closed this Aug 28, 2024
@pillo79 pillo79 deleted the pr-init-arrays branch October 2, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: llext Linkable Loadable Extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants