rtld: Simplify walking program headers

[jhb]

Summary

Store phnum in Obj_Entry instead of phsize and use that to simplify
the terminate expressions when iterating over program headers.

Obtained from: CheriBSD
Sponsored by: AFRL, DARPA

[jhb]

libexec/rtld-elf/powerpc/reloc.c

369

The main thing that inspired this change was simplifying these statements back to a single line after the clang-format pass, but I also find it more readable without all the casts.

[emaste]

I wonder if we want to add some validity checking on ph->p_memsz while we're here? I think a wrong p_memsz will be handled the same before/after this change so it's fine from that perspective, it's more general pondering about what if any validation rtld ought to do.

[kib]

I think this is fine, but for completeness we probably should check that ehdr->e_phentsize == sizeof(Elf_Phdr), and return if not.

[jhb]

In D54710#1249895, @emaste wrote:

I wonder if we want to add some validity checking on ph->p_memsz while we're here? I think a wrong p_memsz will be handled the same before/after this change so it's fine from that perspective, it's more general pondering about what if any validation rtld ought to do.

Well, to be clear, we aren't making any more assumptions in this change. Either we were just multiplying e_phnum by the phdr size to compute phsize, or we just saved p_memsz and then divided by the phdr size each time we used it (without checking for a remainder), and in the new code we we are just hoisting the divide up earlier.

[jhb]

In D54710#1249909, @kib wrote:

I think this is fine, but for completeness we probably should check that ehdr->e_phentsize == sizeof(Elf_Phdr), and return if not.

Yes. I do think we already check AT_PHENT, but might not be checking the size in the ehdr. Granted, that's a bug in the current code, and I might fix it in a separate commit.

[emaste]

we aren't making any more assumptions in this change

Indeed, it's just that looking at this change made me wonder if we ought to add validation.

[kib]

In D54710#1249915, @jhb wrote:

In D54710#1249909, @kib wrote:

I think this is fine, but for completeness we probably should check that ehdr->e_phentsize == sizeof(Elf_Phdr), and return if not.

Yes. I do think we already check AT_PHENT, but might not be checking the size in the ehdr. Granted, that's a bug in the current code, and I might fix it in a separate commit.

Ok.

[jhb]

In D54710#1249915, @jhb wrote:

In D54710#1249909, @kib wrote:

I think this is fine, but for completeness we probably should check that ehdr->e_phentsize == sizeof(Elf_Phdr), and return if not.

Yes. I do think we already check AT_PHENT, but might not be checking the size in the ehdr. Granted, that's a bug in the current code, and I might fix it in a separate commit.

So we already check e_phentsize in check_elf_header in map_object.c which covers anything mmap'd by rtld. For AT_PHENT we assert() that the size is correct for obj_main here:

	} else { /* Main program already loaded. */
		dbg("processing main program's program header");
		assert(aux_info[AT_PHDR] != NULL);
		phdr = (const Elf_Phdr *)aux_info[AT_PHDR]->a_un.a_ptr;
		assert(aux_info[AT_PHNUM] != NULL);
		phnum = aux_info[AT_PHNUM]->a_un.a_val;
		assert(aux_info[AT_PHENT] != NULL);
		assert(aux_info[AT_PHENT]->a_un.a_val == sizeof(Elf_Phdr));
		assert(aux_info[AT_ENTRY] != NULL);
		imgentry = (caddr_t)aux_info[AT_ENTRY]->a_un.a_ptr;
		if ((obj_main = digest_phdr(phdr, phnum, imgentry, argv0)) ==
		    NULL)
			rtld_die();
	}

The one thing I think we don't currently check is e_phentsize for obj_rtld. We could perhaps assert() it in init_rtld here:

	ehdr = (Elf_Ehdr *)mapbase;
	objtmp.phdr = (Elf_Phdr *)((char *)mapbase + ehdr->e_phoff);
	objtmp.phnum = ehdr->e_phnum;

but that is the only place missing I think. (And you'd think the kernel would be checking phentsize of an interpreter as well which we do in __elfN(check_header) in imgact_elf.c.)