kallsyms: Prevent invalid access when showing module buildid #6337
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Author
|
Upstream branch: 026bcf9 |
Author
|
Upstream branch: 3249e8a |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1022518=>bpf-next
branch
from
fa31736 to
c876cb2
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
4eb9670 to
e1b05fd
Compare
The function kallsyms_lookup_buildid() initializes the given @namebuf
by clearing the first and the last byte. It is not clear why.
The 1st byte makes sense because some callers ignore the return code
and expect that the buffer contains a valid string, for example:
- function_stat_show()
- kallsyms_lookup()
- kallsyms_lookup_buildid()
The initialization of the last byte does not make much sense because it
can later be overwritten. Fortunately, it seems that all called
functions behave correctly:
- kallsyms_expand_symbol() explicitly adds the trailing '\0'
at the end of the function.
- All *__address_lookup() functions either use the safe strscpy()
or they do not touch the buffer at all.
Document the reason for clearing the first byte. And remove the useless
initialization of the last byte.
Reviewed-by: Aaron Tomlin <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
…lookup_buildid() The @modname and @modbuildid optional return parameters are set only when the symbol is in a module. Always initialize them so that they do not need to be cleared when the module is not in a module. It simplifies the logic and makes the code even slightly more safe. Note that bpf_address_lookup() function will get updated in a separate patch. Signed-off-by: Petr Mladek <[email protected]>
Author
|
Upstream branch: f1d8c65 |
Add a helper function for reading the optional "build_id" member of struct module. It is going to be used also in ftrace_mod_address_lookup(). Use "#ifdef" instead of "#if IS_ENABLED()" to match the declaration of the optional field in struct module. Reviewed-by: Daniel Gomez <[email protected]> Reviewed-by: Petr Pavlu <[email protected]> Signed-off-by: Petr Mladek <[email protected]>
Put the code for appending the optional "buildid" into a helper function, It makes __sprint_symbol() better readable. Also print a warning when the "modname" is set and the "buildid" isn't. It might catch a situation when some lookup function in kallsyms_lookup_buildid() does not handle the "buildid". Use pr_*_once() to avoid an infinite recursion when the function is called from printk(). The recursion is rather theoretical but better be on the safe side. Signed-off-by: Petr Mladek <[email protected]>
bpf_address_lookup() has been used only in kallsyms_lookup_buildid(). It was supposed to set @modname and @modbuildid when the symbol was in a module. But it always just cleared @modname because BPF symbols were never in a module. And it did not clear @modbuildid because the pointer was not passed. The wrapper is not longer needed. Both @modname and @modbuildid are newly always initialized to NULL in kallsyms_lookup_buildid(). Remove the wrapper and rename __bpf_address_lookup() to bpf_address_lookup() because this variant is used everywhere. Fixes: 9294523 ("module: add printk formats to add module build ID to stacktraces") Signed-off-by: Petr Mladek <[email protected]> Acked-by: Alexei Starovoitov <[email protected]>
__sprint_symbol() might access an invalid pointer when kallsyms_lookup_buildid() returns a symbol found by ftrace_mod_address_lookup(). The ftrace lookup function must set both @modname and @modbuildid the same way as module_address_lookup(). Fixes: 9294523 ("module: add printk formats to add module build ID to stacktraces") Reviewed-by: Aaron Tomlin <[email protected]> Acked-by: Steven Rostedt (Google) <[email protected]> Signed-off-by: Petr Mladek <[email protected]>
kallsyms_lookup_buildid() copies the symbol name into the given buffer so that it can be safely read anytime later. But it just copies pointers to mod->name and mod->build_id which might get reused after the related struct module gets removed. The lifetime of struct module is synchronized using RCU. Take the rcu read lock for the entire __sprint_symbol(). Reviewed-by: Aaron Tomlin <[email protected]> Signed-off-by: Petr Mladek <[email protected]>
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1022518=>bpf-next
branch
from
c876cb2 to
77ac7a4
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
e1b05fd to
b1af442
Compare
Author
|
At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1022518 irrelevant now. Closing PR. |
kernel-patches-daemon-bpf-rc
bot
deleted the
series/1022518=>bpf-next
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request for series with
subject: kallsyms: Prevent invalid access when showing module buildid
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1022518