Skip to content

Support Randomization of Stack Padding Bytes - #1673

Open
Will Hawkins (hawkinsw) wants to merge 3 commits into
microsoft:mainfrom
hawkinsw:randomize_stack_padding_bytes
Open

Will Hawkins (hawkinsw) wants to merge 3 commits into
microsoft:mainfrom
hawkinsw:randomize_stack_padding_bytes

Conversation

@hawkinsw

Copy link
Copy Markdown

In the ELF loader, support optionally initializing the stack with random padding bytes.

@hawkinsw

Copy link
Copy Markdown
Author

I think that Litebox is so cool! I noticed a TODO and thought I would take a crack at it! I tried to be a good citizen, but I am sure that I have missed something from the contributing guidelines in the PR. So, if there is anything I can do to improve it, please just let me know!

In particular, I chose:

  • to use the getrandom crate because it was already used elsewhere in the project; and
  • to surface the choice to override the default stack padding as high as possible to keep from introducing dependencies on external crates into the core libraries.

Again, if this change is something that is helpful but does not follow all the best practices, please let me know! I would love to clean it up.

Thanks again for this great effort!

@hawkinsw

Copy link
Copy Markdown
Author

Praveen K Paladugu (@praveen-pk) Thank you for triggering the CI run! Obviously there is a change in the publicly-visible interfaces. If that is okay, just let me know how to handle the semver check/changes.

If you would prefer to handle supporting randomized padding bytes without exposing the option to the user, that is absolutely ok! Just let me know how I can spruce up the PR!

Thank you again for tiggering the CI run!

@praveen-pk

Copy link
Copy Markdown
Contributor

Will Hawkins (@hawkinsw), this PR looks good to me!

as to semver checks, I believe you only have to add some details to the commit message.

Jay Bosamiya (Microsoft) (@jaybosamiya-ms) could you please help with the semver failure on this PR?

@jaybosamiya-ms Jay Bosamiya (Microsoft) (jaybosamiya-ms) left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, some changes are needed before we can consider merging this, mostly we don't want to be pulling this in as a dependency in this crate. The semver failure, if it still remains, can be fixed right before we consider merging it, please don't worry about it. The Windows failure needs to be fixed in the code however.

Comment thread litebox_runner_linux_userland/Cargo.toml Outdated
Comment thread Cargo.lock Outdated
@hawkinsw

Copy link
Copy Markdown
Author

Thank you Jay Bosamiya (Microsoft) (@jaybosamiya-ms) and Praveen K Paladugu (@praveen-pk) for the reviews! I am terribly sorry for the slow response! I should have an update today! Thank you for being so welcoming to a new contributor!

In the ELF loader, support optionally initializing the stack with
random padding bytes.
@hawkinsw

Copy link
Copy Markdown
Author

Jay Bosamiya (Microsoft) (@jaybosamiya-ms) Using the platform's fill_bytes_crng looks so much nicer! I hope that the updated PR was what you expected. Thanks again for letting me participate!

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