This is an archive of the discontinued LLVM Phabricator instance.

[Object] Fix handling of Elf_Nhdr with sh_addralign=8
ClosedPublic

Authored by MaskRay on May 6 2023, 12:54 AM.

Details

Summary

The generic ABI says:

Padding is present, if necessary, to ensure 8 or 4-byte alignment for the next note entry (depending on whether the file is a 64-bit or 32-bit object). Such padding is not included in descsz.

Our parsing code currently aligns n_namesz. Fix the bug by aligning the start
offset of the descriptor instead. This issue has been benign because the primary
uses of sh_addralign=8 notes are .note.gnu.property, where
sizeof(Elf_Nhdr) + sizeof("GNU") = 16 (already aligned by 8).

In practice, many 64-bit systems incorrectly use sh_addralign=4 notes.
We can use sh_addralign (= p_align) to decide the descriptor padding.
Treat an alignment of 0 and 1 as 4. This approach matches modern GNU readelf
(since 2018).

We have a few tests incorrectly using sh_addralign=0. We may make our behavior
stricter after fixing these tests.

Linux kernel dumped core files use p_align=0 notes, so we need to support the
case for compatibility.

Diff Detail

Event Timeline

MaskRay created this revision.May 6 2023, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 12:54 AM
MaskRay requested review of this revision.May 6 2023, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 12:54 AM
jhenderson added inline comments.May 9 2023, 2:15 AM
llvm/include/llvm/Object/ELF.h
319

This will permit p_align values of 2 and 3, and treat them as 4. Should we instead reject those too? The error message also is not techincally accurate ("is not 4 or 8"), since it could be 0 (or 1/2/3) and be handled by this code. I'm okay with that not changing if we are excluding other values, for convenience of wording though.

325

Nit: this probably wants to be a uint64_t rather than size_t max, right (since p_align could be uint64_t and therefore a larger type on 32-bit hosts than size_t)?

344

Nit: missing ".", and I think the tendency is for "TODO: ", i.e. with colon.

The duplication between this and the Phdr version of this makes me sad :( it feels like we could share the code and therefore avoid duplicating these extra checks.

llvm/include/llvm/Object/ELFTypes.h
647

Is 4 here correct?

llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s
21

Changing the logic means that there is no test coverage that hits the "<corrupted GNU_PROPERTY_TYPE_0>" output, as far as I can tell. Do we need a new test to cover that? It may be worth looking at note-gnu-property.s to see if that already covers what the new test covers.

llvm/tools/llvm-readobj/ELFDumper.cpp
5827

Similar to the earlier comment, should this be uint64_t? Same goes for other cases in the file.

MaskRay updated this revision to Diff 520735.May 9 2023, 9:59 AM
MaskRay marked 4 inline comments as done.

address comments

llvm/include/llvm/Object/ELF.h
325

We have excluded larger p_align values, so using size_t is fine. ELFTypes.h Elf_Nhdr_Impl::getSize returns a size_t and the choice is to match its return value.

llvm/tools/llvm-readobj/ELFDumper.cpp
5827

This should be fine, since we've excluded large p_align values and reported an error very early.

jhenderson accepted this revision.May 10 2023, 12:11 AM

Looks good.

llvm/include/llvm/Object/ELF.h
325

Okay, fair enough.

This revision is now accepted and ready to land.May 10 2023, 12:11 AM
This revision was automatically updated to reflect the committed changes.