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

lazy ksymbols: fix address search #283

Merged

Conversation

NDStrahilevitz
Copy link
Contributor

@NDStrahilevitz NDStrahilevitz commented Jan 5, 2023

ksymbols may sometimes not be sorted at the edges, usually for non system symbols. As such searching by binary search may fail. Add a reverse linear search at the end to catch these symbols.

In commit 3ea92310a0, a new KernelSymbolsTable implementation was added
which uses lazy querying and parsing of a stored string copy of the
/proc/kallsyms file.

In order to keep efficiency of non cached queries, binary search was
used for querying symbols by address, since the file mostly stores the
symbols in order of addresses.

However, on kernels compiled with CONFIG_KALLSYMS_ALL=y, symbols from
other sections may be present in a non ordered place. This wasn't
taken in consideration in initial implementations, and so a query by
address could have failed despite existing.
These symbols usually appear near the end of the file, so a reverse
linear search is added after a failed binary search.

@geyslan
Copy link
Member

geyslan commented Jan 5, 2023

What about to sort fileLines in the lazyKernelSymbols.Refresh() just after the reading? Perhaps we can stick with the binary search only.

geyslan
geyslan previously approved these changes Jan 5, 2023
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM.

@geyslan
Copy link
Member

geyslan commented Jan 5, 2023

What about to sort fileLines in the lazyKernelSymbols.Refresh() just after the reading? Perhaps we can stick with the binary search only.

In addition to that possibility, I think that, in the future, we can refactor a bit by moving some of the parsing logic into a function.

geyslan
geyslan previously approved these changes Jan 5, 2023
@rafaeldtinoco rafaeldtinoco added this to the next-bug-fixes-milestone milestone Jan 6, 2023
@rafaeldtinoco
Copy link
Contributor

Could you explain better in the git log what was the fix you did with this commit? Also, you recommended a PR in tracee to use lazy symbols, if it is broken it is better to update that PR as well (asking for the author to include this fix). Thanks!

@rafaeldtinoco rafaeldtinoco self-requested a review January 6, 2023 04:17
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

Still need to review, but asked for a better git log.

In commit 3ea9231, a new KernelSymbolsTable implementation was added
which uses lazy querying and parsing of a stored string copy of the
/proc/kallsyms file.

In order to keep efficiency of non cached queries, binary search was
used for querying symbols by address, since the file mostly stores the
symbols in order of addresses.

However, on kernels compiled with CONFIG_KALLSYMS_ALL=y, symbols from
other sections may be present in a non ordered place. This wasn't
taken in consideration in initial implementations, and so a query by
address could have failed despite existing.
These symbols usually appear near the end of the file, so a reverse
linear search is added after a failed binary search.
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

LGTM

@rafaeldtinoco rafaeldtinoco merged commit 5ede01b into aquasecurity:main Jan 9, 2023
@NDStrahilevitz NDStrahilevitz deleted the fix_lazy_address_search branch January 9, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants