db_search_symbol: prevent pollution from bogus symbols
ClosedPublic
Actions

Authored by vangyzen on Oct 21 2020, 8:09 PM.

Details

Reviewers

rang_acm.org
bdrewery
markj
dab

Commits

rS367059: db_search_symbol: prevent pollution from bogus symbols

Summary

Assembly files often provide bogus symbols with very small values.
This pollution confounds disassembly by replacing object offsets
with bogus symbols. Since the kernel will never map anything in
the first page, do not query the symbol table for an address smaller
than the page size.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34328
Build 31456: arc lint + arc unit

Event Timeline

vangyzen created this revision.Oct 21 2020, 8:09 PM

Herald added subscribers: dab, badger, imp. · View Herald TranscriptOct 21 2020, 8:09 PM

Harbormaster completed remote builds in B34309: Diff 78563.Oct 21 2020, 8:09 PM

vangyzen requested review of this revision.Oct 21 2020, 8:09 PM

vangyzen added reviewers: rang_acm.org, bdrewery, markj.Oct 21 2020, 8:10 PM

vangyzen edited the summary of this revision. (Show Details)

Seems reasonable, although I think the explanation from your description of this change should really go in the code comment rather than the rather terse and enigmatic "Prevent pollution from bogus symbols".

dab added a reviewer: dab.Oct 22 2020, 1:55 AM

In D26895#599937, @dab wrote:

Seems reasonable, although I think the explanation from your description of this change should really go in the code comment rather than the rather terse and enigmatic "Prevent pollution from bogus symbols".

I agree. It would also be nice to see an example of where the preexisting logic goes wrong.

This looks fine to me. I agree that the comment could be less terse, perhaps something like

The kernel will never map the first page, so any symbols in that range cannot refer to addresses.
Some third-party assembly files define internal constants which appear in their symbol table.
Avoiding the lookup for those symbols avoids replacing small offsets with those symbols
during disassembly. */

The particular instance I noticed this time around was from Intel's ISA-L_crypto library, at
https://github.com/01org/isa-l_crypto.git; the line "rot34 equ 23" caused ddb to disassemble
some operands as, e.g., "rot34+4(%rax)" when that file was included in a kernel build.

This revision is now accepted and ready to land.Oct 22 2020, 2:06 PM

borrow Anton's comment

This revision now requires review to proceed.Oct 22 2020, 7:50 PM

Harbormaster completed remote builds in B34328: Diff 78617.Oct 22 2020, 7:50 PM

So the comparison with PAGE_SIZE is arbitrary, right? There's no reason the aforementioned code could emit symbols with a value between, say, PAGE_SIZE and PAGE_SIZE * 2?

This revision is now accepted and ready to land.Oct 23 2020, 2:58 PM

In D26895#600356, @markj wrote:

So the comparison with PAGE_SIZE is arbitrary, right? There's no reason the aforementioned code could emit symbols with a value between, say, PAGE_SIZE and PAGE_SIZE * 2?

That's correct. We've only _seen_ values that small, so I was taking a cautious approach. Maybe KERNBASE would be more appropriate?

rang_acm.org accepted this revision.Oct 24 2020, 6:18 AM

In D26895#600361, @vangyzen wrote:

In D26895#600356, @markj wrote:

So the comparison with PAGE_SIZE is arbitrary, right? There's no reason the aforementioned code could emit symbols with a value between, say, PAGE_SIZE and PAGE_SIZE * 2?

That's correct. We've only _seen_ values that small, so I was taking a cautious approach. Maybe KERNBASE would be more appropriate?