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

pe: offer basic section manipulations #381

Closed

Conversation

RaitoBezarius
Copy link
Contributor

@RaitoBezarius RaitoBezarius commented Oct 3, 2023

Writing new structures from an existing PE is non-trivial because PE structures
may require a complete rewrite of all the structures and recompute of the offsets for space
reasons.

We sidestep the difficulty by having a meta Writer structure which tracks the information required
to re-render an existing PE information + extra information like new sections.

Depends on #361.

TODO:

  • cleanup code
  • sprinkle / debug / traces everywhere
  • detect when unsupported features are used (symbol table, etc.)
  • handle/write relocations properly
  • handle data directories better
  • re-implement Write support for PE binaries #361 write support in terms of that new writer structure
  • make the example show a more interesting example
  • verify alignments issues
  • canonical test should be load a PE in memory, add a section, reload it and verify it parses well
  • order .debug after certificate table (exception from PE specification…)

@RaitoBezarius RaitoBezarius force-pushed the pe-sections-manipulation branch 2 times, most recently from 7b5f7f1 to 976de6a Compare October 4, 2023 19:18
@RaitoBezarius
Copy link
Contributor Author

Whew! The section adder works!

It is now easy to peek into the data of a data directory and
list all directories with their originally parsed offsets.
We offer a way to build section table which will be queued in a "writer structure" that will know
about pending sections to write.
@RaitoBezarius RaitoBezarius force-pushed the pe-sections-manipulation branch 4 times, most recently from 73629a7 to bd31ce4 Compare January 5, 2024 02:13
Writing new structures from an existing PE is non trivial because PE structures
may require a complete rewrite of all the structures and recompute of the offsets for space
reasons.

We sidestep the difficulty by having a meta Writer structure which tracks the information required
to re-render an existing PE information + extra informations like new sections.
> 216/232  8 Reserved, must be zero

according to the table mentioned in https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-data-directories-image-only

It does not make sense to explode on it therefore.
It is useful to compute debug directory data offsets from RVAs.
@RaitoBezarius RaitoBezarius force-pushed the pe-sections-manipulation branch from bd31ce4 to eacb26a Compare January 5, 2024 03:40
@RaitoBezarius
Copy link
Contributor Author

OK, it's in a not so-bad state, I can boot PE binaries manipulated by this code adding a bunch of sections.

@RaitoBezarius
Copy link
Contributor Author

RaitoBezarius commented Jan 5, 2024

I will move further sanity checking and ordering to another PR if that's possible. I think it's much easier to take this for a walk to discover ideas on how to test it rather than devise testing here. (I found a few bugs by just using it in https://github.com/nix-community/lanzaboote and using our UEFI integration testing).

I can confirm this PR makes all our integration testing pass, this means that the binaries assembled with this section manipulator are quite good.

My plan for follow up are:

After that, anyone who is kind of interested can come and bring COFF support and we will have capabilities to write compilers :).

Unrelated but I have plan to do maybe an ELF writer for a rewrite of https://github.com/NixOS/patchelf in Rust.

@m4b How do you see this PR? Could you give me what you would like to see to be addressed/directions on how to get this merged? Thank you so much for your help!

@m4b
Copy link
Owner

m4b commented Jan 8, 2024

@m4b How do you see this PR? Could you give me what you would like to see to be addressed/directions on how to get this merged? Thank you so much for your help!

I haven't looked in depth yet, in general it looks fine though.

However, since we just did a release:

  1. I would maybe plan what you envision getting merged for a theoretical 0.9
  2. on that topic, I would try to minimize (or completely remove) breaking changes; I understand some of this might not be possible, but if it isn't, I would really encourage changes made to be future proof as much as possible/include as much as you think you'll need (but not more :) )

I would really like to release your changes as a minor patch, if possible, to increase release cadence. But again, if you have breaking changes that must go in/have a good compelling design reason for, that's ok too, just try to batch these changes and plan subsequent PRs soon enough so that all breaking changes go into 0.9

The back context of this is I think i would like to release 1.0 (i've been saying this for years, I know), but i think it is about time; so with that in mind, perhaps try to design your PRs and changes so that if you have to add something/change something later, it will not be a breaking change.

Lastly, and i have not checked whether it's true, but your writer implementation may better be served as it's own crate, if it doesn't depend on any internal/private apis. That would allow you to release a change with whatever cadence and breaking schedule you like, similar to e.g., faerie.

For similar reasons, if the Pwrite changes were done well, it should ideally be possible to only use goblin's public apis to write a PE writer/object writer crate, and it shouldn't be a requirement for the writer to live in goblin (though i'm not opposed to this either).

Anyway, that's a lot of text, but to summarize:

  1. try to plan your PRs with a 1.0 release in mind, and all that entails
  2. batch breaking changes for a 0.9 release, and let me know how many PRs you envision will be in the 0.9 release
  3. minimize if possible, breaking changes, unless you need them
  4. have fun :)

@RaitoBezarius
Copy link
Contributor Author

Closing in favor of #389.

@m4b m4b closed this Mar 11, 2024
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.

2 participants