This is an archive of the discontinued LLVM Phabricator instance.

[ELF] .note.gnu.property: error for invalid pr_datasize
ClosedPublic

Authored by MaskRay on Aug 23 2020, 11:33 AM.

Details

Summary

A n_type==NT_GNU_PROPERTY_TYPE_0 note encodes a program property.
If pr_datasize is invalid, LLD may crash
(https://github.com/ClangBuiltLinux/linux/issues/1141)

This patch adds the error checking, supports big-endian, and some tests
for invalid n_descsz.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 23 2020, 11:33 AM
MaskRay requested review of this revision.Aug 23 2020, 11:33 AM
MaskRay updated this revision to Diff 287267.Aug 23 2020, 11:48 AM

n_descsz is too larget -> data is too short

MaskRay updated this revision to Diff 287268.Aug 23 2020, 12:04 PM

Report the offset of .note.gnu.property

Overall looking good. I think there may be an endianness problem with reinterpret_cast although I could easily be wrong without testing (I don't think we have a target that uses .note.gnu.property with big-endian support in LLD).

lld/ELF/InputFiles.cpp
791–792

Will this work if the Host is little endian and the data in big-endian format?

793

We may be able to be more precise with the size as I think data.size() = sizeof(Elf_Nhdr) + nhdr->getSize()

grimar added inline comments.Aug 24 2020, 2:55 AM
lld/ELF/InputFiles.cpp
791–792

I believe yes. ELF_Nhdr is Elf_Nhdr_Impl:

template <class ELFT>
struct Elf_Nhdr_Impl {
  LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
  Elf_Word n_namesz;
  Elf_Word n_descsz;
  Elf_Word n_type;
...

And Elf_Word s here are using Word = packed<uint32_t> which respects
endianess:

struct packed_endian_specific_integral {
...
operator value_type() const {
  return endian::read<value_type, endian, alignment>(Ptr);
}
template <typename value_type, std::size_t alignment>
inline value_type read(const void *memory, endianness endian) {
  value_type ret;

  memcpy(&ret,
         LLVM_ASSUME_ALIGNED(
             memory, (detail::PickAlignment<value_type, alignment>::value)),
         sizeof(value_type));
  return byte_swap<value_type>(ret, endian);
}
template <typename value_type>
inline value_type byte_swap(value_type value, endianness endian) {
  if ((endian != native) && (endian != system_endianness()))
    sys::swapByteOrder(value);
  return value;
}
814

2 points:

  1. You should be able to use read32 from Target.h:
inline uint32_t read32(const void *p) {
  return llvm::support::endian::read32(p, config->endianness);
}
  1. It probably would read slightly better if it was:
if (desc.size() < 8)
  reportFatal(...);

uint32_t type = read32(desc.data());
uint32_t size = read32(desc.data() + 4);
desc = desc.drop_front(8);

if (desc.size() < size)
  reportFatal(desc, "program property is too short");

i.e
a) I'd not start reading the size + type when the data is truncated (just like the code on the left).
b) Early dropping of 8 bytes at start should allow to avoid the need of + 8 in the code below.

grimar added inline comments.Aug 24 2020, 3:01 AM
lld/ELF/InputFiles.cpp
814

I'd not start reading the size + type when the data is truncated

The code indeed does not starts reading them, I meant I'd just return earlier when it is already known that we can't read the header.

MaskRay marked 3 inline comments as done.Aug 24 2020, 8:52 AM
MaskRay added inline comments.
lld/ELF/InputFiles.cpp
793

This is precise: nhdr->getSize() = sizeof(Elf_Nhdr) + n_descsz.

814
  1. This file does not include Target.h. I don't want to additional introduce an inter-module dependency. I think read32 with an explicit template argument is equally readable (slightly more efficient in performance and better codegen).
  1. Changed.
MaskRay updated this revision to Diff 287412.Aug 24 2020, 8:53 AM
MaskRay marked an inline comment as done.

Address comments

grimar accepted this revision.Aug 25 2020, 1:26 AM

LGTM. Lets see what Peter think.

This revision is now accepted and ready to land.Aug 25 2020, 1:26 AM

LGTM too, I'm happy my comments were addressed, thanks.

This revision was automatically updated to reflect the committed changes.