[PATCH] makedumpfile: Fix several issues with reading ELF pages

[Atsushi Kumagai]

Hello Petr,

Thanks for your investigation and fixing the issue.
I'll merge this into v1.6.0.

Regards,
Atsushi Kumagai

>While adopting the algorithm for libkdumpfile, several corner cases
>were found by a test case:
>
>  1. If the last part of a page is not present in the ELF file, it
>     should be replaced with zeroes. However, the check is incorrect.
>
>  2. If the beginning of a page is not present, following data is
>     read from an incorrect file offset.
>
>  3. If the page is split among several segments, the current
>     position in the buffer is not always taken into account.
>
>Case 1 is a simple typo/braino (writing bufptr instead of endp).
>
>To fix cases 2 and 3, it is best to update the paddr variable so that
>it always corresponds to the current read position in the buffer.
>
>I have tested the new code with a specially crafted ELF dump where one
>page had the following (artificial) layout:
>
>  #  PAGE RANGE    STORED IN   DATA
>  --------------------------------------------------
>  1  0x000-0x007   nowhere     fake zero
>  2  0x008-0x067   LOAD #1     read from file
>  3  0x068-0x06f   nowhere     fake zero
>  4  0x070-0x13f   LOAD #2     read from file
>  5  0x140-0x147   LOAD #2     zero (memsz > filesz)
>  6  0x148-0xff7   LOAD #3     read from file
>  7  0xff8-0xfff   nowhere     fake zero
>
>Case 1 tests the conditional right after get_pt_load_extents().
>Case 2 tests file read after missing data.
>Case 3 tests the conditional from case 1 with non-zero buffer position.
>Case 5 tests the last conditional in the loop (p < endp).
>Case 6 tests exact match in get_pt_load_extents().
>Case 7 tests the final conditional after the loop.
>
>Signed-off-by: Petr Tesarik <ptesarik at suse.com>
>
>---
> makedumpfile.c |   18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -656,14 +656,15 @@ readpage_elf(unsigned long long paddr, v
> 	p = bufptr;
> 	endp = p + info->page_size;
> 	while (p < endp) {
>-		idx = closest_pt_load(paddr + (p - bufptr), endp - p);
>+		idx = closest_pt_load(paddr, endp - p);
> 		if (idx < 0)
> 			break;
>
> 		get_pt_load_extents(idx, &phys_start, &phys_end, &offset, &size);
>-		if (phys_start > paddr + (p - bufptr)) {
>+		if (phys_start > paddr) {
> 			memset(p, 0, phys_start - paddr);
> 			p += phys_start - paddr;
>+			paddr = phys_start;
> 		}
>
> 		offset += paddr - phys_start;
>@@ -677,6 +678,7 @@ readpage_elf(unsigned long long paddr, v
> 				return FALSE;
> 			}
> 			p += size;
>+			paddr += size;
> 		}
> 		if (p < endp) {
> 			size = phys_end - paddr;
>@@ -684,6 +686,7 @@ readpage_elf(unsigned long long paddr, v
> 				size = endp - p;
> 			memset(p, 0, size);
> 			p += size;
>+			paddr += size;
> 		}
> 	}
>
>@@ -691,7 +694,7 @@ readpage_elf(unsigned long long paddr, v
> 		ERRMSG("Attempt to read non-existent page at 0x%llx.\n",
> 		       paddr);
> 		return FALSE;
>-	} else if (p < bufptr)
>+	} else if (p < endp)
> 		memset(p, 0, endp - p);
>
> 	return TRUE;
>@@ -708,14 +711,15 @@ readpage_elf_parallel(int fd_memory, uns
> 	p = bufptr;
> 	endp = p + info->page_size;
> 	while (p < endp) {
>-		idx = closest_pt_load(paddr + (p - bufptr), endp - p);
>+		idx = closest_pt_load(paddr, endp - p);
> 		if (idx < 0)
> 			break;
>
> 		get_pt_load_extents(idx, &phys_start, &phys_end, &offset, &size);
>-		if (phys_start > paddr + (p - bufptr)) {
>+		if (phys_start > paddr) {
> 			memset(p, 0, phys_start - paddr);
> 			p += phys_start - paddr;
>+			paddr = phys_start;
> 		}
>
> 		offset += paddr - phys_start;
>@@ -730,6 +734,7 @@ readpage_elf_parallel(int fd_memory, uns
> 				return FALSE;
> 			}
> 			p += size;
>+			paddr += size;
> 		}
> 		if (p < endp) {
> 			size = phys_end - paddr;
>@@ -737,6 +742,7 @@ readpage_elf_parallel(int fd_memory, uns
> 				size = endp - p;
> 			memset(p, 0, size);
> 			p += size;
>+			paddr += size;
> 		}
> 	}
>
>@@ -744,7 +750,7 @@ readpage_elf_parallel(int fd_memory, uns
> 		ERRMSG("Attempt to read non-existent page at 0x%llx.\n",
> 		       paddr);
> 		return FALSE;
>-	} else if (p < bufptr)
>+	} else if (p < endp)
> 		memset(p, 0, endp - p);
>
> 	return TRUE;