support for images with a single rwx PT_LOAD program header

[lucian]

Normally add-on images have two PT_LOAD program headers:

• a rx PT_LOAD header --> .text
• a rw PT_LOAD header --> .data

Some add-on images may only have a single rwx PT_LOAD header (a concatenation of .text and .data).

[lucian]

[fixed] elf support for images with a single rwx PT_LOAD program header

[lucian]

Please ignore the first patch, it's incomplete. Sorry :(

[stippi]

Deleted the incomplete patch.

[stippi]

Patch looks fine to me, but I'll better leave this judgement to a kernel dev. There is a spelling mistake however, it's textSectionWritable, not textSectionWrittable.

[bonefish]

The problem I see is that for PPC the data segments are executable, too. This patch would break loading them. I guess the first run through the program headers could be used to find out what the situation is.

[lucian]

Ok, on PPC the data segments may be executable, but are there two PT_LOAD program headers: one that is .data (rwx) and .text?

Initially, for me, the single PT_LOAD program header was loaded in .data and nothing in .text.

The problem appeared when it tried to run elf_parse_dynamic_section because of

image->dynamic_section += image->text_region.delta

from load_kernel_add_on() in src/system/kernel/elf.cpp.

If there is only a single rwx .data section and no .text section, image->text_region.delta will be 0 => the dynamic section will not begin after the .text but after NULL. This will most surely result in a page fault.

[bonefish]

Replying to lucian:

Ok, on PPC the data segments may be executable, but are there two PT_LOAD program headers: one that is .data (rwx) and .text?

Yes, e.g.:

bonefish@backbone:~/develop/haiku/haiku/generated-ppc> readelf --segments objects/haiku/ppc/release/add-ons/kernel/bus_managers/pci/pci

Elf file type is DYN (Shared object file)
Entry point 0x3f270
There are 3 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x00000000 0x00000000 0xa5c50 0xa5c50 R E 0x1000
  LOAD           0x0a5c50 0x000a6c50 0x000a6c50 0x464d0 0x46678 RWE 0x1000
  DYNAMIC        0x0a5c6c 0x000a6c6c 0x000a6c6c 0x000c0 0x000c0 RW  0x4

 Section to Segment mapping:
  Segment Sections...
   00     .hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .text .fini .rodata
   01     .eh_frame .ctors .dtors .data.rel.ro .dynamic .data .got .sdata .sbss .plt .bss
   02     .dynamic

Initially, for me, the single PT_LOAD program header was loaded in .data and nothing in .text.

The problem appeared when it tried to run elf_parse_dynamic_section because of

image->dynamic_section += image->text_region.delta

from load_kernel_add_on() in src/system/kernel/elf.cpp.

If there is only a single rwx .data section and no .text section, image->text_region.delta will be 0 => the dynamic section will not begin after the .text but after NULL. This will most surely result in a page fault.

I understand the problem and I'm fine in principle with the patch, but it should be adjusted so that things continue to work on PPC.

[lucian]

I tested the latest patch on x86 with modules that have a single RWE PT_LOAD header and two normal PT_LOAD headers.

I assume this will work on PPC too, but it wouldn't hurt to try :)

[bonefish]

Replying to lucian:

I tested the latest patch on x86 with modules that have a single RWE PT_LOAD header and two normal PT_LOAD headers.

I assume this will work on PPC too, but it wouldn't hurt to try :)

With this patch when booting Haiku in qemu I get:

bfs: mounted "Haiku" (root node at 131072, device = /dev/disk/ata/0/master/raw)
Mounted boot partition: /dev/disk/ata/0/master/raw
module: Search for file_cache/launch_speedup/v1 failed.
amiga_rdb: weird program header flags 0x6
vm_soft_fault: va 0x814ca000 not covered by area in address space
vm_page_fault: vm_soft_fault returned error 'Bad address' on fault at 0x814ca090, ip 0x8004bfcf, write 0, user 0, thread 0xb
PANIC: vm_page_fault: unhandled page fault in kernel space at 0x814ca090, ip 0x8004bfcf

Welcome to Kernel Debugging Land...
Thread 11 "main2" running on CPU 0
stack trace for thread 11 "main2"
    kernel stack: 0x8106b000 to 0x8106f000
frame               caller     <image>:function + offset
 0 8106e798 (+  32) 8010b70e   <kernel_x86> arch_debug_stack_trace() + 0x0012
 1 8106e7b8 (+  16) 8007a1cf   <kernel_x86> stack_trace_trampoline__FPv() + 0x000b
 2 8106e7c8 (+  12) 80110faa   <kernel_x86> arch_debug_call_with_fault_handler() + 0x001b
 3 8106e7d4 (+  48) 8007bc83   <kernel_x86> debug_call_with_fault_handler() + 0x005b
 4 8106e804 (+  64) 8007a3f3   <kernel_x86> kernel_debugger_loop__FPCcT0Pcl() + 0x021f
 5 8106e844 (+  48) 8007a754   <kernel_x86> kernel_debugger_internal__FPCcT0Pcl() + 0x0048
 6 8106e874 (+  48) 8007bff4   <kernel_x86> panic() + 0x0024
 7 8106e8a4 (+  64) 800ef6a1   <kernel_x86> vm_page_fault() + 0x0135
 8 8106e8e4 (+  80) 8010d07e   <kernel_x86> page_fault_exception__FP6iframe() + 0x017a
 9 8106e934 (+  12) 80111f5d   <kernel_x86> int_bottom() + 0x003d
kernel iframe at 0x8106e940 (end = 0x8106e990)
 eax 0x814c8000     ebx 0x0             ecx 0x8207f140   edx 0x814ca090
 esi 0xffffffff     edi 0x80efe600      ebp 0x8106e998   esp 0x8106e974
 eip 0x8004bfcf  eflags 0x210246
 vector: 0xe, error code: 0x0
10 8106e940 (+  88) 8004bfcf   <kernel_x86> elf_parse_dynamic_section__FP14elf_image_info() + 0x002f
11 8106e998 (+ 128) 8004dc59   <kernel_x86> load_kernel_add_on() + 0x0491
12 8106ea18 (+  48) 80053f49   <kernel_x86> load_module_image__FPCcPP12module_image() + 0x006d
13 8106ea48 (+  48) 800541dd   <kernel_x86> get_module_image__FPCcPP12module_image() + 0x0059
14 8106ea78 (+ 160) 80054bb0   <kernel_x86> iterator_get_next_module__FP15module_iteratorPcPUl() + 0x0424
15 8106eb18 (+  64) 8005721a   <kernel_x86> read_next_module_name() + 0x0046
16 8106eb58 (+  80) 800aa858   <kernel_x86> _RescanDiskSystems__Q38BPrivate10DiskDevice18KDiskDeviceManagerRQ48BPrivate10DiskDevice18KDiskDeviceManager13DiskSystemMapb() + 0x00a0
17 8106eba8 (+ 112) 800aa9b2   <kernel_x86> RescanDiskSystems__Q38BPrivate10DiskDevice18KDiskDeviceManager() + 0x0036
18 8106ec18 (+ 864) 800d0846   <kernel_x86> vfs_mount_boot_file_system() + 0x02be
19 8106ef78 (+  96) 80053c04   <kernel_x86> main2__FPv() + 0x00a8
20 8106efd8 (+  32) 8006a4b3   <kernel_x86> _create_kernel_thread_kentry__Fv() + 0x001b
21 8106eff8 (+2130251784) 8006a450   <kernel_x86> thread_kthread_exit__Fv() + 0x0000

Haven't tried to investigate what the issue is.

That aside, some coding style remarks:

@@ -2066,6 +2068,7 @@
 			B_PAGE_SIZE);
 		if (end > reservedSize)
 			reservedSize = end;
+		countExecutableHeaders += programHeaders[i].IsExecutable();

Adding a bool to an int is really not beautiful.

@@ -2105,7 +2108,14 @@
 		}

 		// we're here, so it must be a PT_LOAD segment
-		if (programHeaders[i].IsReadWrite()) {
+
+		// Usually add-ons have two PT_LOAD headers: one for .data one or .text.
+		// x86 and PPC may differ in permission bits for .data's PT_LOAD header
+		//   x86 is usually RW, PPC is RWE

Weird indentation in the comment.

+
+		// Some add-ons may have .text and .data concatenated in a single
+		// PT_LOAD RWE header and we must map that to .text.
+		if (programHeaders[i].IsReadWrite() && (countExecutableHeaders > 1)) {

Superfluous parenthesis.

@@ -2122,6 +2132,10 @@
 			}
 			region = &image->text_region;

+			// some programs may have .text and .data concatenated in a
+			// single PT_LOAD section which is readable/writtable/executable
+			textSectionWritable = programHeaders[i].IsReadWrite();

Typo: "writtable"

@@ -2191,9 +2205,12 @@
 		goto error5;

 	// We needed to read in the contents of the "text" area, but
-	// now we can protect it read-only/execute
+	// now we can protect it read-only/execute, unless this is a
+	// special image with concatenated .text and .data, when it
+	// will also nead write access.

Typo: "nead"

Index: headers/private/system/elf32.h
===================================================================
--- headers/private/system/elf32.h	(revision 37412)
+++ headers/private/system/elf32.h	(working copy)
@@ -367,7 +367,7 @@
 inline bool
 Elf32_Phdr::IsExecutable() const
 {
-	return (p_flags & PF_PROTECTION_MASK) == (PF_READ | PF_EXECUTE);
+	return !!(p_flags & PF_EXECUTE);
 }

Please use (p_flags & PF_EXECUTE) != 0 respective ... == 0 to test bits. Older Haiku code omits the != 0 respectively uses the ! operator, but new/touched code shouldn't.

[lucian]

Replying to bonefish:

Haven't tried to investigate what the issue is.

No need. I sent the wrong patch.

+
+		// Some add-ons may have .text and .data concatenated in a single
+		// PT_LOAD RWE header and we must map that to .text.
+		if (programHeaders[i].IsReadWrite() && (countExecutableHeaders > 1)) {

Superfluous parenthesis.

It wasn't meant to be superfluous. I had another check here but forgot to write it the first time, then fixed it, but sent the first patch :( Sorry for wasting your time.

Please use (p_flags & PF_EXECUTE) != 0 respective ... == 0 to test bits. Older Haiku code omits the != 0 respectively uses the ! operator, but new/touched code shouldn't.

I got inspired by the code above this one. Remark duly noted.

[bonefish]

Thanks, that looks more correct indeed. Applied in hrev37415 with small coding style corrections:

+		if (programHeaders[i].IsExecutable())
+			executableHeaderCount ++;

No space for unary operators (only for binary/trinary ones).

+		if (programHeaders[i].IsReadWrite() &&
+			(!programHeaders[i].IsExecutable() || executableHeaderCount > 1)) {
[...]
 	set_area_protection(image->text_region.id,
-		B_KERNEL_READ_AREA | B_KERNEL_EXECUTE_AREA);
+		B_KERNEL_READ_AREA | B_KERNEL_EXECUTE_AREA |
+		(textSectionWritable ? B_KERNEL_WRITE_AREA : 0));

Operators at the beginning of the next line instead at the end of the previous one.