Page MenuHomeFreeBSD

Add a tool to modify control features in ELF binaries
ClosedPublic

Authored by emaste on Feb 22 2019, 1:00 AM.

Details

Summary

Add a tool to modify control features in ELF binaries.

Submitted by: Bora Ozarslan borako.ozarslan@gmail.com

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste added subscribers: kib, markj.Apr 17 2019, 5:01 PM
kib added inline comments.Apr 17 2019, 5:33 PM
tools/tools/controlelf/Makefile
4 ↗(On Diff #54208)

This is not needed, right ?

6 ↗(On Diff #54208)

And this.

23 ↗(On Diff #54208)

Why this is needed ? Perhaps you should use #include "" instead of <>.

24 ↗(On Diff #54208)

And what is this ?

27 ↗(On Diff #54208)

Bump WARNS to max, and perhaps make warnings an error. New code should be clean.

tools/tools/controlelf/controlelf.1
1 ↗(On Diff #54208)

Really ?

tools/tools/controlelf/controlelf.c
160 ↗(On Diff #54208)

Extra blank line.

161 ↗(On Diff #54208)

if (elf != NULL)

164 ↗(On Diff #54208)

Checking for error from close(2) is almost always pointless, perhaps except for EBADF.

186 ↗(On Diff #54208)

Blank line after '{' if no locals.

191 ↗(On Diff #54208)

Use bool's ?

199 ↗(On Diff #54208)

return (0);

200 ↗(On Diff #54208)

I do not understand this loop, in particular the condition inside the if().

212 ↗(On Diff #54208)

No initializers in local declarations.

226 ↗(On Diff #54208)

nitems()

245 ↗(On Diff #54208)

Excess ().

261 ↗(On Diff #54208)

return on the next line.

315 ↗(On Diff #54208)

Check for error from lseek.

319 ↗(On Diff #54208)

Check errors from read, check for short reads.

326 ↗(On Diff #54208)

roundup2().

327 ↗(On Diff #54208)

sizeof(char) is sillyness. Why do you need calloc at all ?

336 ↗(On Diff #54208)

This is yet another incorrect use of strncmp. At least you must check that strlen(name) == 7.

345 ↗(On Diff #54208)

Why this cast ? It looks arbitrary.

362 ↗(On Diff #54208)

branch statement on the next line.

emaste added inline comments.Apr 18 2019, 8:32 PM
tools/tools/controlelf/controlelf.c
200 ↗(On Diff #54208)

if (gelf_getphdr(elf, i, phdr) == NULL) probably

212 ↗(On Diff #54208)

FWIW this is part of style(9) I dislike, but it is what it is.

emaste added inline comments.Apr 22 2019, 2:42 PM
tools/tools/controlelf/controlelf.1
24–25 ↗(On Diff #54208)

These two lines should be replaced with just:

.\" $FreeBSD$
borako.ozarslan_gmail.com marked 26 inline comments as done.Apr 22 2019, 4:21 PM
borako.ozarslan_gmail.com added inline comments.
tools/tools/controlelf/controlelf.c
200 ↗(On Diff #54208)

Essentially the for loop is going through each program header and finding the one that is of type PT_NOTE and returning that. In libelf, the recommended way to enumerate through the program headers is this way. The program header is returned inside the phdr which is given as a parameter. The function gelf_getphdr gets the ith program header and replaces the values inside the phdr. On success it returns phdr (the address given to it) and on failure it returns NULL. So that's what the first if is checking for, if the function succeeded or not. Then the second if checks if this program header is of type PT_NOTE. I now see that there may be more than 1 PT_NOTE header.

Fixed Makefile.
Changed code to comply with style(9).
Some cleanup.
Check all PT_NOTE headers until one that has feature control note has been found.
Updated copyright notices.
Get rid of control features that haven't been implemented yet (they are in review). Get rid of all mentions/examples of it.

emaste commandeered this revision.Jul 26 2019, 4:14 PM
emaste updated this revision to Diff 60170.
emaste edited reviewers, added: borako.ozarslan_gmail.com; removed: emaste.

initial updates incorporating kib feedback

This revision was not accepted when it landed; it landed in state Needs Review.Sep 27 2019, 4:28 PM
This revision was automatically updated to reflect the committed changes.