Skip to content
This repository was archived by the owner on Jan 10, 2025. It is now read-only.

elf: fail validation if writable .data sections are present#240

Merged
alessandrod merged 2 commits intosolana-labs:mainfrom
alessandrod:writable-data-sections
Dec 14, 2021
Merged

elf: fail validation if writable .data sections are present#240
alessandrod merged 2 commits intosolana-labs:mainfrom
alessandrod:writable-data-sections

Conversation

@alessandrod
Copy link
Copy Markdown

This extends the check we already do for .bss to all writable .data sections. Improves error messages for programs that use mutable global state, for which before we were returning access violation errors due to relocated addresses being invalid, eg:

Access violation in program section at address 0x1000a39b0 of size 8 by instruction #5065

Versus the error we provide now:

Found writable section (.data._ZN3std6thread8ThreadId3new7COUNTER17h88fd3fcacf16583cE.llvm.14438380808032992799) in ELF, read-write data not supported

The new error can actually help folks pin point the issue with their code (in my example, some hashmap code pulling in threading code).

@alessandrod
Copy link
Copy Markdown
Author

One thing to note is that doing verification this way is obviously stricter, as it will fail at validation time vs run time when the data gets written to. But we've already decided to do it this way for .bss so I don't think it makes sense to treat writable .data sections differently.

Copy link
Copy Markdown

@dmakarov dmakarov left a comment

Choose a reason for hiding this comment

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

thank you for adding the tests in addition to the fix!

Comment thread src/elf.rs Outdated
This extends the check we already do for .bss to all writable .data sections.
Improves error messages for programs that use mutable global state, for which
before we were returning access violation errors due to relocated addresses
being invalid, eg:

	Access violation in program section at address 0x1000a39b0 of size 8 by instruction #5065

Versus the error we provide now:

	Found writable section (.data._ZN3std6thread8ThreadId3new7COUNTER17h88fd3fcacf16583cE.llvm.14438380808032992799) in ELF, read-write data not supported
@alessandrod alessandrod force-pushed the writable-data-sections branch from e63c81d to 21b3239 Compare December 13, 2021 08:13
@alessandrod alessandrod requested a review from Lichtso December 13, 2021 08:13
@alessandrod
Copy link
Copy Markdown
Author

I guess when this get merged I should bump version and tag?

alessandrod added a commit to alessandrod/solana that referenced this pull request Dec 13, 2021
When reject_writable_data_sections is turned on, programs with writable data
sections (global mutable state) are rejected.

See solana-labs/rbpf#240
@alessandrod
Copy link
Copy Markdown
Author

This is the corresponding solana PR: solana-labs/solana#21826. Marked as draft for now since we need to bump rbpf version before it can get in.

@dmakarov
Copy link
Copy Markdown

I guess when this get merged I should bump version and tag?

Yes, pushing a tag should publish the new version on crates.io automatically. If this fails, please, let me know.

@Lichtso
Copy link
Copy Markdown

Lichtso commented Dec 13, 2021

But we still have to bump the version manually. Only the cargo publish happens automatically, right?

@dmakarov
Copy link
Copy Markdown

But we still have to bump the version manually. Only the cargo publish happens automatically, right?

Correct. A new version tag still needs to be pushed manually to the github repository to trigger the publishing of a new version to crates.io.

@jackcmay jackcmay mentioned this pull request Dec 13, 2021
@jackcmay jackcmay requested a review from dmakarov December 13, 2021 23:39
@jackcmay
Copy link
Copy Markdown

@alessandrod I added a few more changes to roll in a missing featurization from earlier, check them out.

@alessandrod
Copy link
Copy Markdown
Author

@alessandrod I added a few more changes to roll in a missing featurization from earlier, check them out.

Looks good!

alessandrod added a commit to alessandrod/solana that referenced this pull request Dec 14, 2021
When reject_writable_data_sections is turned on, programs with writable data
sections (global mutable state) are rejected.

See solana-labs/rbpf#240
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants