[llvm-readelf] - Do not crash when the PT_INTERP has an invalid offset.

[grimar]

Summary

We do not verify the p_offset of the PT_INTERP header and tool may
crash when a program interpreter name string goes past the end of the file.

Depends on D78805.

[MaskRay]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

316

Isn't the test covered by ERROR-INTERP2?

llvm/tools/llvm-readobj/ELFDumper.cpp

4154

The two lines can be joined.

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

316

Nope. They have different error messages. In this case the offset points past the EOF. In ERROR-INTERP2 it is withing the file space.

[grimar]

Going to merge 2 tests

[grimar]

• Merged 2 test cases.

[MaskRay]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

326

The main problem with the test is that if yaml2obj layout is updated, the p_offset fields will need an update which may be a bit tricky. Since the file size is displayed, this is probably fine.

560 = size(Ehdr) + size(Phdrs) + section contents with padding + size(Shdrs) = 64 + 56*5 + alignTo(1+0x13, 8) + 64*3 = 560

[MaskRay]

a broken offset.

an invalid offset

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

316

used in the YAML below.

326

You could improve things slightly with a bit of cleverness in FileCheck, although I don't know whether it's a good idea. I think it should work as described, possibly with a few tweaks:

Firstly, emit the wc output to a file, and concatenate the llvm-readelf program header dump to the same file. Then, use FileCheck's numeric variables to capture the size and re-use it:

# RUN: wc -c < %t.err > %t.err.out
# RUN: FileCheck %s --check-prefix=SIZE --input-file=%t.err.out
# SIZE: 560
## Optionally use echo to add some extra data here that you can check to avoid matching something else.

## Write the additional 'C', '\0, 'C' bytes to the end.
# RUN: echo -n -e "C\x00C" >> %t.err

# RUN: llvm-readelf --program-headers %t.err 2>&1 >> %t.err.out
# RUN: FileCheck %s --input-file=%t.err.out -DFILE=%t.err --check-prefix=ERROR-INTERP

# ERROR-INTERP: [[#SIZE:]]
## If you add the extra data, check it here.
# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         [[#SIZE]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
# ERROR-INTERP-NEXT: INTERP         [[#SIZE+1]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: ]

and so on.

This has the benefit of only needing to change the YAML and one single check, should the layout change. You could even avoid the file concatenation etc by just capturing the variable based on the first INTERP's offset, if you want.

349

is a null

llvm/tools/llvm-readobj/ELFDumper.cpp

4132

Maybe rename to ReportBadInterp or something. ReportError is a bit confusing when it's not an error!

[grimar]

• Addressed review comments.

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

326

You could even avoid the file concatenation etc by just capturing the variable based on the first INTERP's offset, if you want.

This looks good I think. Thanks for suggestion!
I've updated the test.

[grimar]

• Rebase properly.

[jhenderson]

LGTM, with one remaining request.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

327

I believe you don't want the leading 0x000 bit, and then you also won't need the %x either. I believe FileCheck will automatically recognise it as hex that way (as things stand, I think it might not, but can't remember the details). If you want ot be sure that it is hex though, you can keep the %x (and might need it in the lines below too to be sure its always hex, but I'm not sure - experimentation needed). FWIW, I don't think it being hex is a necessary check for this specific case, since we check it earlier.

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

327

I believe you don't want the leading 0x000 bit, and then you also won't need the %x either.

It does not work for me, this was the only way I've managed to make this work.

The following fail:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         [[#OFFSET:]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
# ERROR-INTERP-NEXT: INTERP         [[#OFFSET + 1]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: ]

D:\Work3\LLVM\llvm-project\llvm\test\tools\llvm-readobj\ELF\gnu-phdrs.test:324:2
2: error: ERROR-INTERP-NEXT: expected string not found in input
# ERROR-INTERP-NEXT: INTERP [[#OFFSET + 1]]
                     ^
<stdin>:10:2: note: scanning from here
 INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
 ^
<stdin>:10:2: note: with "OFFSET + 1" equal to "1"
 INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
 ^

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

327

Ah, okay, I guess the 0x is not cosnidered part of the numeric pattern and the default only reads decimal. @thopre, is that correct?

Anyway, from my experiments the leading zeroes can be included, and please specify '%x' explicitly in the definition.

332

Don't forget to update 0x232 to 0x[[#OFFSET+2]] and similar in the messages below.

[grimar]

• Addressed comments in according to latest discussion. (I am not sure I got everything right).

(It still depends on D78805.)

I am leaving on vacation until 12 of May.

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

327

Just in case to confirm that my understanding about "leading zeroes" was correct here,
here is a result of my experiments:

The following works:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x230]]

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x000230]]

The following doesn't:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:0x230]]

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:230]]

So we need the "0x000[[" part, but do not need leading zeroes for OFFSET.

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

332

Don't forget to update 0x232 to 0x[[#OFFSET+2]] and similar in the messages below.

Now I am not sure we want it. I guess it will accept both 0x0000000000232 and just 0x232, does not seem we want it.

[jhenderson]

LGTM, with the two error message fixes. I'd appreciate @thopre's input on the numeric variable usage too, but that shouldn't block this landing.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

327

I missed the bit where you were explicitly specifying the value to capture and didn't replicate it in my own experiments properly. I'm at a loss here. The problem is that if the layout changes to have fewer (or more) leading zeroes, it will fail due to that.

That seems to be missing the point of numeric variables. Leading zeroes should be able to be included in the match.

334

Can you not get rid of the %x in all the usages? I thought that should be derived from the definition of OFFSET if unspecified and only needed specifying if different from the original.

Also, there are still two instances of explicit "0x233" in these messages.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

332

I think it's okay for either. I don't think it matters whether we have leading zeroes or not in the error messages, personally. Also, as we've seen above, the leading zeroes aren't matched anyway, right? ;)

[grimar]

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

332

Also, as we've seen above, the leading zeroes aren't matched anyway, right? ;)

Right. I've just confused myself.

[grimar]

Changes Planned -> Accepted, since it was accepted. Will commit shortly with requested changes.

[thopre]

In D79013#2014703, @jhenderson wrote:

LGTM, with the two error message fixes. I'd appreciate @thopre's input on the numeric variable usage too, but that shouldn't block this landing.

%x and %X deliberately don't match the 0x0* prefix to allow:

• matching plain hex number without 0x prefix
• testing the actual number of leading zeros in case a tool use leading zero to align numbers for instance

It might be useful to introduce a precision syntax a la printf to specify how many leading zeros are allowed, as well as a --canonicalize-numvar-leading-zero or better named option which would allow matching as many leading zeros. Leading zeros are allowed in immediates though since that has no impact on what is matched, hence why #FOO+0x00018 is allowed.

Let me know if you need further clarification.

[grimar]

Was committed as 0d54612164519c874eecf5a1be68597b96410628, phab did not close it for some reason. And does not allow me to close it normally.

[grimar]

• Close it normally.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test

 ## Check how llvm-readelf dumps program headers and prints sections to segments mapping.
 
 ## Check that -l, --program-headers and --segments are the same option.
-# RUN: yaml2obj -DBITS=32 -DMACHINE=EM_386 %s -o %t32.elf
+# RUN: yaml2obj --docnum=1 -DBITS=32 -DMACHINE=EM_386 %s -o %t32.elf
 # RUN: llvm-readelf -l %t32.elf 2>&1 > %t.readelf-l.txt
 # RUN: llvm-readelf --program-headers %t32.elf 2>&1 > %t.readelf-pheaders.txt
 # RUN: cmp %t.readelf-l.txt %t.readelf-pheaders.txt
 # RUN: llvm-readelf --segments %t32.elf 2>&1 > %t.readelf-segments.txt
 # RUN: cmp %t.readelf-l.txt %t.readelf-segments.txt
 
 ## Check we are able to print program headers of different types.
 # RUN: llvm-readelf -l %t32.elf | \
 # RUN:   FileCheck %s --check-prefixes=ELF32,MAPPING --strict-whitespace --match-full-lines
 
-# RUN: yaml2obj -DBITS=64 -DMACHINE=EM_X86_64 %s -o %t64.elf
+# RUN: yaml2obj --docnum=1 -DBITS=64 -DMACHINE=EM_X86_64 %s -o %t64.elf
 # RUN: llvm-readelf -l %t64.elf | \
 # RUN:   FileCheck %s --check-prefixes=ELF64,MAPPING --strict-whitespace --match-full-lines
 
 #       ELF32:There are 23 program headers, starting at offset 52
 # ELF32-EMPTY:
 # ELF32-NEXT:Program Headers:
 # ELF32-NEXT:  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
 # ELF32-NEXT:  PHDR           0x000314 0x00001000 0x00001000 0x00003 0x00003  W  0x1
Sections:
       - Section: .foo.end
 
 ## Check that --section-mapping=false --program-headers produces just program headers.
 ## See more tests for interactions with '--section-mapping' in gnu-section-mapping.test.
 # RUN: llvm-readelf --section-mapping=false --program-headers %t64.elf \
 # RUN:   | FileCheck %s --check-prefix=ELF64 --implicit-check-not="Section to Segment mapping:"
 
 ## Check how we dump ARM specific program headers.
-# RUN: yaml2obj -DBITS=64 -DMACHINE=EM_ARM %s -o %tarm.elf
+# RUN: yaml2obj --docnum=1 -DBITS=64 -DMACHINE=EM_ARM %s -o %tarm.elf
 # RUN: llvm-readelf --program-headers %tarm.elf | FileCheck %s --check-prefix=ARM
 
 # ARM:      <unknown>: 0x70000000 0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 # ARM-NEXT:            EXIDX      0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 # ARM-NEXT: <unknown>: 0x70000002 0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 
 ## Check how we dump MIPS specific program headers.
-# RUN: yaml2obj -DBITS=64 -DMACHINE=EM_MIPS %s -o %tmips.elf
+# RUN: yaml2obj --docnum=1 -DBITS=64 -DMACHINE=EM_MIPS %s -o %tmips.elf
 # RUN: llvm-readelf --program-headers %tmips.elf | FileCheck %s --check-prefix=MIPS
 
 # MIPS:      REGINFO  0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 # MIPS-NEXT: RTPROC   0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 # MIPS-NEXT: OPTIONS  0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
 # MIPS-NEXT: ABIFLAGS 0x000548 0x0000000000001000 0x0000000000001000 0x000003 0x000003 0x1
+
+## Check we report a warning when a program interpreter name is non-null-terminated or when
+## PT_INTERP has an offset that goes past the end of the file.
+# RUN: yaml2obj --docnum=2 %s -o %t.err
+
+## Show the size of the output produced. It is used below.
+# RUN: wc -c < %t.err | FileCheck %s --check-prefix=SIZE
+# SIZE: 560
+
+## Write the additional 'C', '\0, 'C' bytes to the end.
+# RUN: echo -n -e "C\x00C" >> %t.err
+
+# RUN: llvm-readelf --program-headers %t.err 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t.err --check-prefix=ERROR-INTERP
+
+# ERROR-INTERP:      Type           Offset
+# ERROR-INTERP-NEXT: INTERP         0x000230
+# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
+# ERROR-INTERP-NEXT: INTERP         0x000231
+# ERROR-INTERP-NEXT:     [Requesting program interpreter: ]
+# ERROR-INTERP-NEXT: INTERP         0x000232
+# ERROR-INTERP-NEXT: warning: '[[FILE]]': unable to read program interpreter name at offset 0x232: it is not null-terminated
+# ERROR-INTERP-NEXT: INTERP         0x000233
+# ERROR-INTERP-NEXT: warning: '[[FILE]]': unable to read program interpreter name at offset 0x233: it goes past the end of the file (0x233)
+# ERROR-INTERP-NEXT: INTERP         0xaabbccddeeff1122
+# ERROR-INTERP-NEXT: warning: '[[FILE]]': unable to read program interpreter name at offset 0xaabbccddeeff1122: it goes past the end of the file (0x233)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+ProgramHeaders:
+## Case 1: the offset points to the first additional byte.
+  - Type:   PT_INTERP
+    Offset: 560
+## Case 1: the offset points to the second additional byte,
+##         which is null byte.
+  - Type:   PT_INTERP
+    Offset: 561
+## Case 3: the offset points to the third additional
+##         byte, which is the last byte in the file.
+  - Type:   PT_INTERP
+    Offset: 562
+## Case 4: the offset goes 1 byte past the end of the file.
+  - Type:   PT_INTERP
+    Offset: 563
+## Case 5: an arbitrary large offset that goes past the end of the file.
+  - Type:   PT_INTERP
+    Offset: 0xAABBCCDDEEFF1122

llvm/tools/llvm-readobj/ELFDumper.cpp

for (const auto &Phdr :
     Fields[3].Str = to_string(format_hex(Phdr.p_paddr, Width));
     Fields[4].Str = to_string(format_hex(Phdr.p_filesz, SizeWidth));
     Fields[5].Str = to_string(format_hex(Phdr.p_memsz, SizeWidth));
     Fields[6].Str = printPhdrFlags(Phdr.p_flags);
     Fields[7].Str = to_string(format_hex(Phdr.p_align, 1));
     for (auto Field : Fields)
       printField(Field);
     if (Phdr.p_type == ELF::PT_INTERP) {
-      OS << "\n      [Requesting program interpreter: ";
-      OS << reinterpret_cast<const char *>(Obj->base()) + Phdr.p_offset << "]";
+      OS << "\n";
+      auto ReportError = [&](const Twine &Msg) {
+        reportWarning(
+            createError("unable to read program interpreter name at offset 0x" +
+                        Twine::utohexstr(Phdr.p_offset) + ": " + Msg),
+            this->FileName);
+      };
+
+      if (Phdr.p_offset >= Obj->getBufSize()) {
+        ReportError("it goes past the end of the file (0x" +
+                    Twine::utohexstr(Obj->getBufSize()) + ")");
+        continue;
+      }
+
+      const char *Data =
+          reinterpret_cast<const char *>(Obj->base()) + Phdr.p_offset;
+      size_t MaxSize = Obj->getBufSize() - Phdr.p_offset;
+      size_t Len = strnlen(Data, MaxSize);
+      if (Len == MaxSize) {
+        ReportError("it is not null-terminated");
+        continue;
+      }
+
+      OS << "      [Requesting program interpreter: ";
+      OS << StringRef(Data, Len) << "]";
     }
     OS << "\n";
   }
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printSectionMapping(const ELFO *Obj) {
   OS << "\n Section to Segment mapping:\n  Segment Sections...\n";