kallsyms: Prevent invalid access when showing module buildid #6297
+76
−28
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: 4cb4897 |
Author
|
Upstream branch: 4cb4897 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
485983b to
4b13616
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
c95b89c to
828eff8
Compare
Author
|
Upstream branch: b54a8e1 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
4b13616 to
6f63320
Compare
Author
|
Upstream branch: b54a8e1 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
6f63320 to
39e2480
Compare
Author
|
Upstream branch: b54a8e1 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
39e2480 to
f488ef2
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
828eff8 to
03f9e36
Compare
Author
|
Upstream branch: b54a8e1 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
f488ef2 to
6055237
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
03f9e36 to
97961e6
Compare
Author
|
Upstream branch: 6f1f4c1 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
6055237 to
53f5c1f
Compare
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
bpf-next_base
branch
from
97961e6 to
2456350
Compare
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. Signed-off-by: Petr Mladek <[email protected]> Reviewed-by: Petr Pavlu <[email protected]> Reviewed-by: Daniel Gomez <[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]>
Make bpf_address_lookup() compatible with module_address_lookup() and clear the pointer to @modbuildid together with @modname. It is not strictly needed because __sprint_symbol() reads @modbuildid only when @modname is set. But better be on the safe side and make the API more safe. Fixes: 9294523 ("module: add printk formats to add module build ID to stacktraces") Signed-off-by: Petr Mladek <[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") Signed-off-by: Petr Mladek <[email protected]> Acked-by: Steven Rostedt (Google) <[email protected]>
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.
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(). Signed-off-by: Petr Mladek <[email protected]>
Author
|
Upstream branch: f8c67d8 |
kernel-patches-daemon-bpf-rc
bot
force-pushed
the
series/1019899=>bpf-next
branch
from
53f5c1f to
bd6d77f
Compare
Author
|
At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1019899 irrelevant now. Closing PR. |
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: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1019899