[PATCH v2 0/4] makedumpfile: harden parsing of old prink buffer

[PATCH v2 0/4] makedumpfile: harden parsing of old prink buffer

[Philipp Rudo]

Hi,

dumping the dmesg can cause an endless loop for the old prink mechanism (>
v3.5.0 and < v5.10.0) when the log_buf got corrupted. This series fixes those
cases by adding a cycle detection. The cycle detection is implemented in a
generic way so that it can be reused in other parts of makedumpfile.

Thanks
Philipp

v2:
	* Rename 'idx' to 'ptr'
	* Also print the non-loop part when a cycle was detected. Such a
	  situation can happen when log_buf wrapped around in the kernel
	  (log_first_idx != 0) and the corruption occurred on an
	  idx < log_first_idx.
	* Add patch 4 fixing a bug independent from the memory corruption but
	  found while investigating it.

Philipp Rudo (4):
  makedumpfile: add generic cycle detection
  makedumpfile: use pointer arithmetics for dump_dmesg
  makedumpfile: use cycle detection when parsing the prink log_buf
  makedumpfile: print error when reading with unsupported compression

 Makefile       |   2 +-
 detect_cycle.c |  99 +++++++++++++++++++++++++++++++++++++
 detect_cycle.h |  40 +++++++++++++++
 makedumpfile.c | 131 ++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 247 insertions(+), 25 deletions(-)
 create mode 100644 detect_cycle.c
 create mode 100644 detect_cycle.h

-- 
2.35.1

[PATCH v2 1/4] makedumpfile: add generic cycle detection

[Philipp Rudo]

In order to work makedumpfile needs to interpret data read from the
dump. This can cause problems as the data from the dump cannot be
trusted (otherwise the kernel wouldn't have panicked in the first
place). This also means that every loop which stop condition depend on
data read from the dump has a chance to loop forever. Thus add a generic
cycle detection mechanism that allows to detect and handle such
situations appropriately.

For cycle detection use Brent's algorithm [1] as it has constant memory
usage. With this it can also be used in the kdump kernel without the
danger that it runs oom when iterating large data structures.
Furthermore it only depends on some pointer arithmetic. Thus the
performance impact (as long as no cycle was detected) should be
comparatively small.

[1] https://en.wikipedia.org/wiki/Cycle_detection#Brent's_algorithm

Suggested-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 Makefile       |  2 +-
 detect_cycle.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++
 detect_cycle.h | 40 ++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 detect_cycle.c
 create mode 100644 detect_cycle.h

diff --git a/Makefile b/Makefile
index f118b31..3441364 100644
--- a/Makefile
+++ b/Makefile
@@ -45,7 +45,7 @@ CFLAGS_ARCH += -m32
 endif
 
 SRC_BASE = makedumpfile.c makedumpfile.h diskdump_mod.h sadump_mod.h sadump_info.h
-SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c cache.c tools.c printk.c
+SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c cache.c tools.c printk.c detect_cycle.c
 OBJ_PART=$(patsubst %.c,%.o,$(SRC_PART))
 SRC_ARCH = arch/arm.c arch/arm64.c arch/x86.c arch/x86_64.c arch/ia64.c arch/ppc64.c arch/s390x.c arch/ppc.c arch/sparc64.c arch/mips64.c
 OBJ_ARCH=$(patsubst %.c,%.o,$(SRC_ARCH))
diff --git a/detect_cycle.c b/detect_cycle.c
new file mode 100644
index 0000000..6b551a7
--- /dev/null
+++ b/detect_cycle.c
@@ -0,0 +1,99 @@
+/*
+ * detect_cycle.c  --  Generic cycle detection using Brent's algorithm
+ *
+ * Created by: Philipp Rudo <prudo@redhat.com>
+ *
+ * Copyright (c) 2022 Red Hat, Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <stdlib.h>
+
+#include "detect_cycle.h"
+
+struct detect_cycle {
+	/* First entry of the list */
+	void *head;
+
+	/* Variables required by Brent's algorithm */
+	void *fast_p;
+	void *slow_p;
+	unsigned long length;
+	unsigned long power;
+
+	/* Function to get the next entry in the list */
+	dc_next_t next;
+
+	/* Private data passed to next */
+	void *data;
+};
+
+struct detect_cycle *dc_init(void *head, void *data, dc_next_t next)
+{
+	struct detect_cycle *new;
+
+	new = malloc(sizeof(*new));
+	if (!new)
+		return NULL;
+
+	new->next = next;
+	new->data = data;
+
+	new->head = head;
+	new->slow_p = head;
+	new->fast_p = head;
+	new->length = 0;
+	new->power  = 2;
+
+	return new;
+}
+
+int dc_next(struct detect_cycle *dc, void **next)
+{
+
+	if (dc->length == dc->power) {
+		dc->length = 0;
+		dc->power *= 2;
+		dc->slow_p = dc->fast_p;
+	}
+
+	dc->fast_p = dc->next(dc->fast_p, dc->data);
+	dc->length++;
+
+	if (dc->slow_p == dc->fast_p)
+		return 1;
+
+	*next = dc->fast_p;
+	return 0;
+}
+
+void dc_find_start(struct detect_cycle *dc, void **first, unsigned long *len)
+{
+	void *slow_p, *fast_p;
+	unsigned long tmp;
+
+	slow_p = fast_p = dc->head;
+	tmp = dc->length;
+
+	while (tmp) {
+		fast_p = dc->next(fast_p, dc->data);
+		tmp--;
+	}
+
+	while (slow_p != fast_p) {
+		slow_p = dc->next(slow_p, dc->data);
+		fast_p = dc->next(fast_p, dc->data);
+	}
+
+	*first = slow_p;
+	*len = dc->length;
+}
diff --git a/detect_cycle.h b/detect_cycle.h
new file mode 100644
index 0000000..2ca75c7
--- /dev/null
+++ b/detect_cycle.h
@@ -0,0 +1,40 @@
+/*
+ * detect_cycle.h  --  Generic cycle detection using Brent's algorithm
+ *
+ * Created by: Philipp Rudo <prudo@redhat.com>
+ *
+ * Copyright (c) 2022 Red Hat, Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+struct detect_cycle;
+
+typedef void *(*dc_next_t)(void *prev, void *data);
+
+/*
+ * Initialize cycle detection.
+ * Returns a pointer to allocated struct detect_cycle. The caller is
+ * responsible to free the memory after use.
+ */
+struct detect_cycle *dc_init(void *head, void *data, dc_next_t next);
+
+/*
+ * Get next entry in the list using dc->next.
+ * Returns 1 when cycle was detected, 0 otherwise.
+ */
+int dc_next(struct detect_cycle *dc, void **next);
+
+/*
+ * Get the start and length of the cycle. Must only be called after cycle was
+ * detected by dc_next.
+ */
+void dc_find_start(struct detect_cycle *dc, void **first, unsigned long *len);
-- 
2.35.1

[PATCH v2 2/4] makedumpfile: use pointer arithmetics for dump_dmesg

[Philipp Rudo]

When parsing the printk buffer for the old printk mechanism (> v3.5.0+ and
< 5.10.0) a log entry is currently specified by the offset into the
buffer where the entry starts. Change this to use a pointers instead.
This is done in preparation for using the new cycle detection mechanism.

Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 makedumpfile.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 7ed9756..9a05c96 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -5482,13 +5482,10 @@ dump_log_entry(char *logptr, int fp, const char *file_name)
  * get log record by index; idx must point to valid message.
  */
 static char *
-log_from_idx(unsigned int idx, char *logbuf)
+log_from_ptr(char *logptr, char *logbuf)
 {
-	char *logptr;
 	unsigned int msglen;
 
-	logptr = logbuf + idx;
-
 	/*
 	 * A length == 0 record is the end of buffer marker.
 	 * Wrap around and return the message at the start of
@@ -5502,14 +5499,13 @@ log_from_idx(unsigned int idx, char *logbuf)
 	return logptr;
 }
 
-static long
-log_next(unsigned int idx, char *logbuf)
+static void *
+log_next(void *_logptr, void *_logbuf)
 {
-	char *logptr;
+	char *logptr = _logptr;
+	char *logbuf = _logbuf;
 	unsigned int msglen;
 
-	logptr = logbuf + idx;
-
 	/*
 	 * A length == 0 record is the end of buffer marker. Wrap around and
 	 * read the message at the start of the buffer as *this* one, and
@@ -5519,10 +5515,10 @@ log_next(unsigned int idx, char *logbuf)
 	msglen = USHORT(logptr + OFFSET(printk_log.len));
 	if (!msglen) {
 		msglen = USHORT(logbuf + OFFSET(printk_log.len));
-		return msglen;
+		return logbuf + msglen;
 	}
 
-	return idx + msglen;
+	return logptr + msglen;
 }
 
 int
@@ -5530,11 +5526,12 @@ dump_dmesg()
 {
 	int log_buf_len, length_log, length_oldlog, ret = FALSE;
 	unsigned long index, log_buf, log_end;
-	unsigned int idx, log_first_idx, log_next_idx;
+	unsigned int log_first_idx, log_next_idx;
 	unsigned long long first_idx_sym;
 	unsigned long log_end_2_6_24;
 	unsigned      log_end_2_6_25;
 	char *log_buffer = NULL, *log_ptr = NULL;
+	char *ptr;
 
 	/*
 	 * log_end has been changed to "unsigned" since linux-2.6.25.
@@ -5681,13 +5678,13 @@ dump_dmesg()
 			ERRMSG("Can't open output file.\n");
 			goto out;
 		}
-		idx = log_first_idx;
-		while (idx != log_next_idx) {
-			log_ptr = log_from_idx(idx, log_buffer);
+		ptr = log_buffer + log_first_idx;
+		while (ptr != log_buffer + log_next_idx) {
+			log_ptr = log_from_ptr(ptr, log_buffer);
 			if (!dump_log_entry(log_ptr, info->fd_dumpfile,
 					    info->name_dumpfile))
 				goto out;
-			idx = log_next(idx, log_buffer);
+			ptr = log_next(ptr, log_buffer);
 		}
 		if (!close_files_for_creating_dumpfile())
 			goto out;
-- 
2.35.1

[PATCH v2 3/4] makedumpfile: use cycle detection when parsing the prink log_buf

[Philipp Rudo]

The old printk mechanism (> v3.5.0 and < v5.10.0) had a fixed size
buffer (log_buf) that contains all messages. The location for the next
message is stored in log_next_idx. In case the log_buf runs full
log_next_idx wraps around and starts overwriting old messages@the
beginning of the buffer. The wraparound is denoted by a message with
msg->len == 0.

Following the behavior described above blindly in makedumpfile is
dangerous as e.g. a memory corruption could overwrite (parts of) the
log_buf. If the corruption adds a message with msg->len == 0 this leads
to an endless loop when dumping the dmesg with makedumpfile appending
the messages up to the corruption over and over again to the output file
until file system is full. Fix this by using cycle detection and aboard
once one is detected.

While at it also verify that the index is within the log_buf and thus
guard against corruptions with msg->len != 0.

Reported-by: Audra Mitchell <aubaker@redhat.com>
Suggested-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 makedumpfile.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 9a05c96..b7ac999 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -15,6 +15,7 @@
  */
 #include "makedumpfile.h"
 #include "print_info.h"
+#include "detect_cycle.h"
 #include "dwarf_info.h"
 #include "elf_info.h"
 #include "erase_info.h"
@@ -5528,10 +5529,11 @@ dump_dmesg()
 	unsigned long index, log_buf, log_end;
 	unsigned int log_first_idx, log_next_idx;
 	unsigned long long first_idx_sym;
+	struct detect_cycle *dc = NULL;
 	unsigned long log_end_2_6_24;
 	unsigned      log_end_2_6_25;
 	char *log_buffer = NULL, *log_ptr = NULL;
-	char *ptr;
+	char *ptr, *next_ptr;
 
 	/*
 	 * log_end has been changed to "unsigned" since linux-2.6.25.
@@ -5679,12 +5681,55 @@ dump_dmesg()
 			goto out;
 		}
 		ptr = log_buffer + log_first_idx;
+		dc = dc_init(ptr, log_buffer, log_next);
 		while (ptr != log_buffer + log_next_idx) {
 			log_ptr = log_from_ptr(ptr, log_buffer);
 			if (!dump_log_entry(log_ptr, info->fd_dumpfile,
 					    info->name_dumpfile))
 				goto out;
 			ptr = log_next(ptr, log_buffer);
+			if (dc_next(dc, (void **) &next_ptr)) {
+				unsigned long len;
+				int in_cycle;
+				char *first;
+
+				/* Clear everything we have already written... */
+				ftruncate(info->fd_dumpfile, 0);
+				lseek(info->fd_dumpfile, 0, SEEK_SET);
+
+				/* ...and only write up to the corruption. */
+				dc_find_start(dc, (void **) &first, &len);
+				ptr = log_buffer + log_first_idx;
+				in_cycle = FALSE;
+				while (len) {
+					log_ptr = log_from_ptr(ptr, log_buffer);
+					if (!dump_log_entry(log_ptr,
+							    info->fd_dumpfile,
+							    info->name_dumpfile))
+						goto out;
+					ptr = log_next(ptr, log_buffer);
+
+					if (log_ptr == first)
+						in_cycle = TRUE;
+
+					if (in_cycle)
+						len--;
+				}
+				ERRMSG("Cycle when parsing dmesg detected.\n");
+				ERRMSG("The printk log_buf is most likely corrupted.\n");
+				ERRMSG("log_buf = 0x%lx, idx = 0x%lx\n", log_buf, ptr - log_buffer);
+				close_files_for_creating_dumpfile();
+				goto out;
+			}
+			if (next_ptr < log_buffer ||
+			    next_ptr > log_buffer + log_buf_len - SIZE(printk_log)) {
+				ERRMSG("Index outside log_buf detected.\n");
+				ERRMSG("The printk log_buf is most likely corrupted.\n");
+				ERRMSG("log_buf = 0x%lx, idx = 0x%lx\n", log_buf, ptr - log_buffer);
+				close_files_for_creating_dumpfile();
+				goto out;
+			}
+			ptr = next_ptr;
 		}
 		if (!close_files_for_creating_dumpfile())
 			goto out;
@@ -5694,6 +5739,7 @@ dump_dmesg()
 out:
 	if (log_buffer)
 		free(log_buffer);
+	free(dc);
 
 	return ret;
 }
-- 
2.35.1

[PATCH v2 4/4] makedumpfile: print error when reading with unsupported compression

[Philipp Rudo]

Currently makedumpfile only checks if the required compression algorithm
was enabled during build when compressing a dump but not when reading
from one. This can lead to situations where, one version of makedumpfile
creates the dump using a compression algorithm an other version of
makedumpfile doesn't support. When the second version now tries to, e.g.
extract the dmesg from the dump it will fail with an error similar to

	# makedumpfile --dump-dmesg vmcore dmesg.txt
	__vtop4_x86_64: Can't get a valid pgd.
	readmem: Can't convert a virtual address(ffffffff92e18284) to physical address.
	readmem: type_addr: 0, addr:ffffffff92e18284, size:390
	check_release: Can't get the address of system_utsname.

	makedumpfile Failed.

That's because readpage_kdump_compressed{_parallel} does not return
with an error if the page it is trying to read is compressed with an
unsupported compression algorithm. Thus readmem copies random data from
the (uninitialized) cachebuf to its caller and thus causing the error
above.

Fix this by checking if the required compression algorithm is supported
in readpage_kdump_compressed{_parallel} and print a proper error message
if it isn't.

Reported-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Philipp Rudo <prudo@redhat.com>
---
 makedumpfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index b7ac999..56f3b6c 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -865,9 +865,14 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+	} else if ((pd.flags & DUMP_DH_COMPRESSED_LZO)) {
 #ifdef USELZO
-	} else if (info->flag_lzo_support
-		   && (pd.flags & DUMP_DH_COMPRESSED_LZO)) {
+		if (!info->flag_lzo_support) {
+			ERRMSG("lzo compression unsupported\n");
+			out = FALSE;
+			goto out_error;
+		}
+
 		retlen = info->page_size;
 		ret = lzo1x_decompress_safe((unsigned char *)buf, pd.size,
 					    (unsigned char *)bufptr, &retlen,
@@ -876,9 +881,14 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("lzo compression unsupported\n");
+		ERRMSG("Try `make USELZO=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
-#ifdef USESNAPPY
 	} else if ((pd.flags & DUMP_DH_COMPRESSED_SNAPPY)) {
+#ifdef USESNAPPY
 
 		ret = snappy_uncompressed_length(buf, pd.size, (size_t *)&retlen);
 		if (ret != SNAPPY_OK) {
@@ -891,14 +901,24 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("snappy compression unsupported\n");
+		ERRMSG("Try `make USESNAPPY=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
-#ifdef USEZSTD
 	} else if ((pd.flags & DUMP_DH_COMPRESSED_ZSTD)) {
+#ifdef USEZSTD
 		ret = ZSTD_decompress(bufptr, info->page_size, buf, pd.size);
 		if (ZSTD_isError(ret) || (ret != info->page_size)) {
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("zstd compression unsupported\n");
+		ERRMSG("Try `make USEZSTD=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
 	}
 
@@ -964,9 +984,14 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+	} else if ((pd.flags & DUMP_DH_COMPRESSED_LZO)) {
 #ifdef USELZO
-	} else if (info->flag_lzo_support
-		   && (pd.flags & DUMP_DH_COMPRESSED_LZO)) {
+		if (!info->flag_lzo_support) {
+			ERRMSG("lzo compression unsupported\n");
+			out = FALSE;
+			goto out_error;
+		}
+
 		retlen = info->page_size;
 		ret = lzo1x_decompress_safe((unsigned char *)buf, pd.size,
 					    (unsigned char *)bufptr, &retlen,
@@ -975,9 +1000,14 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("lzo compression unsupported\n");
+		ERRMSG("Try `make USELZO=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
-#ifdef USESNAPPY
 	} else if ((pd.flags & DUMP_DH_COMPRESSED_SNAPPY)) {
+#ifdef USESNAPPY
 
 		ret = snappy_uncompressed_length(buf, pd.size, (size_t *)&retlen);
 		if (ret != SNAPPY_OK) {
@@ -990,14 +1020,24 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("snappy compression unsupported\n");
+		ERRMSG("Try `make USESNAPPY=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
-#ifdef USEZSTD
 	} else if ((pd.flags & DUMP_DH_COMPRESSED_ZSTD)) {
+#ifdef USEZSTD
 		ret = ZSTD_decompress(bufptr, info->page_size, buf, pd.size);
 		if (ZSTD_isError(ret) || (ret != info->page_size)) {
 			ERRMSG("Uncompress failed: %d\n", ret);
 			goto out_error;
 		}
+#else
+		ERRMSG("zstd compression unsupported\n");
+		ERRMSG("Try `make USEZSTD=on` when building.\n");
+		out = FALSE;
+		goto out_error;
 #endif
 	}
 
-- 
2.35.1

[PATCH v2 0/4] makedumpfile: harden parsing of old prink buffer

[David Wysochanski]

On Mon, Mar 14, 2022 at 12:04 PM Philipp Rudo <prudo@redhat.com> wrote:
>
> Hi,
>
> dumping the dmesg can cause an endless loop for the old prink mechanism (>
> v3.5.0 and < v5.10.0) when the log_buf got corrupted. This series fixes those
> cases by adding a cycle detection. The cycle detection is implemented in a
> generic way so that it can be reused in other parts of makedumpfile.
>
> Thanks
> Philipp
>
> v2:
>         * Rename 'idx' to 'ptr'
>         * Also print the non-loop part when a cycle was detected. Such a
>           situation can happen when log_buf wrapped around in the kernel
>           (log_first_idx != 0) and the corruption occurred on an
>           idx < log_first_idx.
>         * Add patch 4 fixing a bug independent from the memory corruption but
>           found while investigating it.
>
> Philipp Rudo (4):
>   makedumpfile: add generic cycle detection
>   makedumpfile: use pointer arithmetics for dump_dmesg
>   makedumpfile: use cycle detection when parsing the prink log_buf
>   makedumpfile: print error when reading with unsupported compression
>
>  Makefile       |   2 +-
>  detect_cycle.c |  99 +++++++++++++++++++++++++++++++++++++
>  detect_cycle.h |  40 +++++++++++++++
>  makedumpfile.c | 131 ++++++++++++++++++++++++++++++++++++++++---------
>  4 files changed, 247 insertions(+), 25 deletions(-)
>  create mode 100644 detect_cycle.c
>  create mode 100644 detect_cycle.h
>
> --
> 2.35.1
>

Thanks for doing v2.  Reviewing / testing this now...

[PATCH v2 0/4] makedumpfile: harden parsing of old prink buffer

[David Wysochanski]

On Wed, Mar 16, 2022 at 9:17 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Mon, Mar 14, 2022 at 12:04 PM Philipp Rudo <prudo@redhat.com> wrote:
> >
> > Hi,
> >
> > dumping the dmesg can cause an endless loop for the old prink mechanism (>
> > v3.5.0 and < v5.10.0) when the log_buf got corrupted. This series fixes those
> > cases by adding a cycle detection. The cycle detection is implemented in a
> > generic way so that it can be reused in other parts of makedumpfile.
> >
> > Thanks
> > Philipp
> >
> > v2:
> >         * Rename 'idx' to 'ptr'
> >         * Also print the non-loop part when a cycle was detected. Such a
> >           situation can happen when log_buf wrapped around in the kernel
> >           (log_first_idx != 0) and the corruption occurred on an
> >           idx < log_first_idx.
> >         * Add patch 4 fixing a bug independent from the memory corruption but
> >           found while investigating it.
> >
> > Philipp Rudo (4):
> >   makedumpfile: add generic cycle detection
> >   makedumpfile: use pointer arithmetics for dump_dmesg
> >   makedumpfile: use cycle detection when parsing the prink log_buf
> >   makedumpfile: print error when reading with unsupported compression
> >
> >  Makefile       |   2 +-
> >  detect_cycle.c |  99 +++++++++++++++++++++++++++++++++++++
> >  detect_cycle.h |  40 +++++++++++++++
> >  makedumpfile.c | 131 ++++++++++++++++++++++++++++++++++++++++---------
> >  4 files changed, 247 insertions(+), 25 deletions(-)
> >  create mode 100644 detect_cycle.c
> >  create mode 100644 detect_cycle.h
> >
> > --
> > 2.35.1
> >
>
> Thanks for doing v2.  Reviewing / testing this now...

You can add
Reviewed-and-tested-by: Dave Wysochanski <dwysocha@redhat.com>

I tested this patchset against a large set of vmcores comparing output
of "makedumpfile --dump-dmesg" with existing makedumpfile
(kexec-tools-2.0.20-46.el8_4.3.x86_64) with the latest upstream plus
these patches.  No difference in output was seen.

As advertised, this handles the loop condition when log_buf is
corrupted.  And with the v2 version of patch 3, the dmesg output is
the same as "crash log" on the same vmcore.  Also verified patch #4
works as advertised - thanks for including a better error message
there for users.

[PATCH v2 2/4] makedumpfile: use pointer arithmetics for dump_dmesg

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> When parsing the printk buffer for the old printk mechanism (> v3.5.0+ and
> < 5.10.0) a log entry is currently specified by the offset into the
> buffer where the entry starts. Change this to use a pointers instead.
> This is done in preparation for using the new cycle detection mechanism.
> 
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
> ---
>  makedumpfile.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 7ed9756..9a05c96 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -5482,13 +5482,10 @@ dump_log_entry(char *logptr, int fp, const char *file_name)
>   * get log record by index; idx must point to valid message.
>   */
>  static char *
> -log_from_idx(unsigned int idx, char *logbuf)
> +log_from_ptr(char *logptr, char *logbuf)
>  {
> -	char *logptr;
>  	unsigned int msglen;
> 
> -	logptr = logbuf + idx;
> -
>  	/*
>  	 * A length == 0 record is the end of buffer marker.
>  	 * Wrap around and return the message at the start of
> @@ -5502,14 +5499,13 @@ log_from_idx(unsigned int idx, char *logbuf)
>  	return logptr;
>  }
> 
> -static long
> -log_next(unsigned int idx, char *logbuf)
> +static void *
> +log_next(void *_logptr, void *_logbuf)
>  {
> -	char *logptr;
> +	char *logptr = _logptr;
> +	char *logbuf = _logbuf;
>  	unsigned int msglen;
> 
> -	logptr = logbuf + idx;
> -

I added the following change to the log_from_ptr() and log_next() because of
- assigning to the argument (possible but unusual)
- the unnecessary local variables

--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -5494,16 +5494,14 @@ log_from_ptr(char *logptr, char *logbuf)
 
        msglen = USHORT(logptr + OFFSET(printk_log.len));
        if (!msglen)
-               logptr = logbuf;
+               return logbuf;
 
        return logptr;
 }
 
 static void *
-log_next(void *_logptr, void *_logbuf)
+log_next(void *logptr, void *logbuf)
 {
-       char *logptr = _logptr;
-       char *logbuf = _logbuf;
        unsigned int msglen;
 
        /*

Thanks,
Kazu

>  	/*
>  	 * A length == 0 record is the end of buffer marker. Wrap around and
>  	 * read the message at the start of the buffer as *this* one, and
> @@ -5519,10 +5515,10 @@ log_next(unsigned int idx, char *logbuf)
>  	msglen = USHORT(logptr + OFFSET(printk_log.len));
>  	if (!msglen) {
>  		msglen = USHORT(logbuf + OFFSET(printk_log.len));
> -		return msglen;
> +		return logbuf + msglen;
>  	}
> 
> -	return idx + msglen;
> +	return logptr + msglen;
>  }
> 
>  int
> @@ -5530,11 +5526,12 @@ dump_dmesg()
>  {
>  	int log_buf_len, length_log, length_oldlog, ret = FALSE;
>  	unsigned long index, log_buf, log_end;
> -	unsigned int idx, log_first_idx, log_next_idx;
> +	unsigned int log_first_idx, log_next_idx;
>  	unsigned long long first_idx_sym;
>  	unsigned long log_end_2_6_24;
>  	unsigned      log_end_2_6_25;
>  	char *log_buffer = NULL, *log_ptr = NULL;
> +	char *ptr;
> 
>  	/*
>  	 * log_end has been changed to "unsigned" since linux-2.6.25.
> @@ -5681,13 +5678,13 @@ dump_dmesg()
>  			ERRMSG("Can't open output file.\n");
>  			goto out;
>  		}
> -		idx = log_first_idx;
> -		while (idx != log_next_idx) {
> -			log_ptr = log_from_idx(idx, log_buffer);
> +		ptr = log_buffer + log_first_idx;
> +		while (ptr != log_buffer + log_next_idx) {
> +			log_ptr = log_from_ptr(ptr, log_buffer);
>  			if (!dump_log_entry(log_ptr, info->fd_dumpfile,
>  					    info->name_dumpfile))
>  				goto out;
> -			idx = log_next(idx, log_buffer);
> +			ptr = log_next(ptr, log_buffer);
>  		}
>  		if (!close_files_for_creating_dumpfile())
>  			goto out;
> --
> 2.35.1

[PATCH v2 4/4] makedumpfile: print error when reading with unsupported compression

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> Currently makedumpfile only checks if the required compression algorithm
> was enabled during build when compressing a dump but not when reading
> from one. This can lead to situations where, one version of makedumpfile
> creates the dump using a compression algorithm an other version of
> makedumpfile doesn't support. When the second version now tries to, e.g.
> extract the dmesg from the dump it will fail with an error similar to
> 
> 	# makedumpfile --dump-dmesg vmcore dmesg.txt
> 	__vtop4_x86_64: Can't get a valid pgd.
> 	readmem: Can't convert a virtual address(ffffffff92e18284) to physical address.
> 	readmem: type_addr: 0, addr:ffffffff92e18284, size:390
> 	check_release: Can't get the address of system_utsname.
> 
> 	makedumpfile Failed.
> 
> That's because readpage_kdump_compressed{_parallel} does not return
> with an error if the page it is trying to read is compressed with an
> unsupported compression algorithm. Thus readmem copies random data from
> the (uninitialized) cachebuf to its caller and thus causing the error
> above.
> 
> Fix this by checking if the required compression algorithm is supported
> in readpage_kdump_compressed{_parallel} and print a proper error message
> if it isn't.
> 
> Reported-by: Dave Wysochanski <dwysocha@redhat.com>
> Signed-off-by: Philipp Rudo <prudo@redhat.com>
> ---
>  makedumpfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index b7ac999..56f3b6c 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -865,9 +865,14 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +	} else if ((pd.flags & DUMP_DH_COMPRESSED_LZO)) {
>  #ifdef USELZO
> -	} else if (info->flag_lzo_support
> -		   && (pd.flags & DUMP_DH_COMPRESSED_LZO)) {
> +		if (!info->flag_lzo_support) {
> +			ERRMSG("lzo compression unsupported\n");
> +			out = FALSE;

I removed those "out = FALSE;" lines because it's already initialized to FALSE.

Thanks,
Kazu

> +			goto out_error;
> +		}
> +
>  		retlen = info->page_size;
>  		ret = lzo1x_decompress_safe((unsigned char *)buf, pd.size,
>  					    (unsigned char *)bufptr, &retlen,
> @@ -876,9 +881,14 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("lzo compression unsupported\n");
> +		ERRMSG("Try `make USELZO=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
> -#ifdef USESNAPPY
>  	} else if ((pd.flags & DUMP_DH_COMPRESSED_SNAPPY)) {
> +#ifdef USESNAPPY
> 
>  		ret = snappy_uncompressed_length(buf, pd.size, (size_t *)&retlen);
>  		if (ret != SNAPPY_OK) {
> @@ -891,14 +901,24 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("snappy compression unsupported\n");
> +		ERRMSG("Try `make USESNAPPY=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
> -#ifdef USEZSTD
>  	} else if ((pd.flags & DUMP_DH_COMPRESSED_ZSTD)) {
> +#ifdef USEZSTD
>  		ret = ZSTD_decompress(bufptr, info->page_size, buf, pd.size);
>  		if (ZSTD_isError(ret) || (ret != info->page_size)) {
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("zstd compression unsupported\n");
> +		ERRMSG("Try `make USEZSTD=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
>  	}
> 
> @@ -964,9 +984,14 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +	} else if ((pd.flags & DUMP_DH_COMPRESSED_LZO)) {
>  #ifdef USELZO
> -	} else if (info->flag_lzo_support
> -		   && (pd.flags & DUMP_DH_COMPRESSED_LZO)) {
> +		if (!info->flag_lzo_support) {
> +			ERRMSG("lzo compression unsupported\n");
> +			out = FALSE;
> +			goto out_error;
> +		}
> +
>  		retlen = info->page_size;
>  		ret = lzo1x_decompress_safe((unsigned char *)buf, pd.size,
>  					    (unsigned char *)bufptr, &retlen,
> @@ -975,9 +1000,14 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("lzo compression unsupported\n");
> +		ERRMSG("Try `make USELZO=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
> -#ifdef USESNAPPY
>  	} else if ((pd.flags & DUMP_DH_COMPRESSED_SNAPPY)) {
> +#ifdef USESNAPPY
> 
>  		ret = snappy_uncompressed_length(buf, pd.size, (size_t *)&retlen);
>  		if (ret != SNAPPY_OK) {
> @@ -990,14 +1020,24 @@ readpage_kdump_compressed_parallel(int fd_memory, unsigned long long paddr,
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("snappy compression unsupported\n");
> +		ERRMSG("Try `make USESNAPPY=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
> -#ifdef USEZSTD
>  	} else if ((pd.flags & DUMP_DH_COMPRESSED_ZSTD)) {
> +#ifdef USEZSTD
>  		ret = ZSTD_decompress(bufptr, info->page_size, buf, pd.size);
>  		if (ZSTD_isError(ret) || (ret != info->page_size)) {
>  			ERRMSG("Uncompress failed: %d\n", ret);
>  			goto out_error;
>  		}
> +#else
> +		ERRMSG("zstd compression unsupported\n");
> +		ERRMSG("Try `make USEZSTD=on` when building.\n");
> +		out = FALSE;
> +		goto out_error;
>  #endif
>  	}
> 
> --
> 2.35.1

[PATCH v2 0/4] makedumpfile: harden parsing of old prink buffer

[HAGIO KAZUHITO =?unknown-8bit?b?6JCp5bC+IOS4gOS7gQ==?=]

-----Original Message-----
> On Wed, Mar 16, 2022 at 9:17 AM David Wysochanski <dwysocha@redhat.com> wrote:
> >
> > On Mon, Mar 14, 2022 at 12:04 PM Philipp Rudo <prudo@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > dumping the dmesg can cause an endless loop for the old prink mechanism (>
> > > v3.5.0 and < v5.10.0) when the log_buf got corrupted. This series fixes those
> > > cases by adding a cycle detection. The cycle detection is implemented in a
> > > generic way so that it can be reused in other parts of makedumpfile.
> > >
> > > Thanks
> > > Philipp
> > >
> > > v2:
> > >         * Rename 'idx' to 'ptr'
> > >         * Also print the non-loop part when a cycle was detected. Such a
> > >           situation can happen when log_buf wrapped around in the kernel
> > >           (log_first_idx != 0) and the corruption occurred on an
> > >           idx < log_first_idx.
> > >         * Add patch 4 fixing a bug independent from the memory corruption but
> > >           found while investigating it.
> > >
> > > Philipp Rudo (4):
> > >   makedumpfile: add generic cycle detection
> > >   makedumpfile: use pointer arithmetics for dump_dmesg
> > >   makedumpfile: use cycle detection when parsing the prink log_buf
> > >   makedumpfile: print error when reading with unsupported compression
> > >
> > >  Makefile       |   2 +-
> > >  detect_cycle.c |  99 +++++++++++++++++++++++++++++++++++++
> > >  detect_cycle.h |  40 +++++++++++++++
> > >  makedumpfile.c | 131 ++++++++++++++++++++++++++++++++++++++++---------
> > >  4 files changed, 247 insertions(+), 25 deletions(-)
> > >  create mode 100644 detect_cycle.c
> > >  create mode 100644 detect_cycle.h
> > >
> > > --
> > > 2.35.1
> > >
> >
> > Thanks for doing v2.  Reviewing / testing this now...
> 
> You can add
> Reviewed-and-tested-by: Dave Wysochanski <dwysocha@redhat.com>

Thank you Pilipp and Dave, for the improvement.

Applied with the small changes I sent.

Thanks,
Kazu


> 
> I tested this patchset against a large set of vmcores comparing output
> of "makedumpfile --dump-dmesg" with existing makedumpfile
> (kexec-tools-2.0.20-46.el8_4.3.x86_64) with the latest upstream plus
> these patches.  No difference in output was seen.
> 
> As advertised, this handles the loop condition when log_buf is
> corrupted.  And with the v2 version of patch 3, the dmesg output is
> the same as "crash log" on the same vmcore.  Also verified patch #4
> works as advertised - thanks for including a better error message
> there for users.