vmcore-dmesg: Do not write beyond end of allocated buffer

scan_vmcoreinfo() currently assumes that every line in vmcoreinfo note ends with \n and overwrites new line character with \0. But last entry in note, CRASHTIME= does not end with \n and this leads to corrupting memory as we write beyond end of buffer.

Normally things were fine but when I added some fields to vmcoreinfo, this bug started showing and vmcore-dmesg started crashing.

I am planning to send a patch to fix this in kernel but it might be good idea to handle this case in user space too so that vmcore-dmesg works fine with cores of older kernels.

Signed-off-by Vivek Goyal <[email protected]>

---
 vmcore-dmesg/vmcore-dmesg.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c
===================================================================

vmcore-dmesg/vmcore-dmesg.c +1 -0

@@ -14,6 +14,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <elf.h>
+#include <stdbool.h>
 
 /* The 32bit and 64bit note headers make it clear we don't care */
 typedef Elf32_Nhdr Elf_Nhdr;

vmcore-dmesg/vmcore-dmesg.c +20 -1

+
 #define SYMBOL(sym) {					\
 	.str = "SYMBOL(" #sym  ")=",			\
 	.name = #sym,					\

@@ -243,7 +247,25 @@ static void scan_vmcoreinfo(char *start,
 		/* Find the end of the current line */
 		for (eol = pos; (eol <= last) && (*eol != '\n') ; eol++)
 			;
-		len = eol - pos + 1;
+		if (eol > last) {
+			/*
+			 * We did not find \n and note ended. Currently kernel
+			 * is appending last field CRASH_TIME without \n. It
+			 * is ugly but handle it.
+			 */
+			eol = last;
+			len = eol - pos + 1;
+			strncpy(temp_buf, pos, len);
+			temp_buf[len + 1] = '\0';
+
+			pos = temp_buf;
+			len = len + 1;
+			eol = pos + len -1;
+			last_line = true;
+		} else  {
+			len = eol - pos + 1;
+		}
+
 		/* Stomp the last character so I am guaranteed a terminating null */
 		*eol = '\0';
 		/* Copy OSRELEASE if I see it */

Quoted 11 lines · Vivek Goyal

scan_vmcoreinfo() currently assumes that every line in vmcoreinfo note ends with \n and overwrites new line character with \0. But last entry in note, CRASHTIME= does not end with \n and this leads to corrupting memory as we write beyond end of buffer.

Normally things were fine but when I added some fields to vmcoreinfo, this bug started showing and vmcore-dmesg started crashing.

I am planning to send a patch to fix this in kernel but it might be good idea to handle this case in user space too so that vmcore-dmesg works fine with cores of older kernels.

Good plan.

Quoted 18 lines

Signed-off-by Vivek Goyal <[email protected]>

---
 vmcore-dmesg/vmcore-dmesg.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c
===================================================================

vmcore-dmesg/vmcore-dmesg.c +1 -0

@@ -14,6 +14,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <elf.h>
+#include <stdbool.h>
 
 /* The 32bit and 64bit note headers make it clear we don't care */
 typedef Elf32_Nhdr Elf_Nhdr;

vmcore-dmesg/vmcore-dmesg.c static void scan_vmcoreinfo(char *start, · line 225

Quoted 5 lines

vmcore-dmesg/vmcore-dmesg.c +2 -0

@@ -220,6 +221,9 @@ static void scan_vmcoreinfo(char *start,
 {
 	char *last = start + size - 1;
 	char *pos, *eol;
+	char temp_buf[1024];
+	bool last_line = false;

Is there a chance of over-running temp_buf?

I would be more comfortable if there was a check below to ensure that len can never be greater than sizeof(temp_buf).

Quoted 5 lines

vmcore-dmesg/vmcore-dmesg.c static void scan_vmcoreinfo(char *start, · line 225

Quoted 2 lines

vmcore-dmesg/vmcore-dmesg.c +2 -0

+	char temp_buf[1024];
+	bool last_line = false;

Is there a chance of over-running temp_buf?

Quoted 2 lines

I would be more comfortable if there was a check below to ensure that len can never be greater than sizeof(temp_buf).

Hi Simon,

Agreed. Making sure temp_buf does not overflow is a good idea. Here is the udpated patch.

Thanks
Vivek

vmcore-dmesg: Do not write beyond end of buffer

scan_vmcoreinfo() currently assumes that every vmcoreinfo note line ends with \n and overwrites new line with \0. But last entry in note, CRASHTIME= does not end with \n and this leads to corrupting memory as we write beyond end of buffer.

Normally things were fine but when I added some fields to vmcoreinfo, this bug started showing and vmcore-dmesg started crashing.

I am planning to send a patch to fix this in kernel but it might be good idea to handle this case in user space too so that vmcore-dmesg works fine with cores of older kernels.

Signed-off-by Vivek Goyal <[email protected]>

---
 vmcore-dmesg/vmcore-dmesg.c |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c
===================================================================

vmcore-dmesg/vmcore-dmesg.c +28 -1

@@ -14,6 +14,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <elf.h>
+#include <stdbool.h>
 
 /* The 32bit and 64bit note headers make it clear we don't care */
 typedef Elf32_Nhdr Elf_Nhdr;

@@ -220,6 +221,9 @@ static void scan_vmcoreinfo(char *start,
 {
 	char *last = start + size - 1;
 	char *pos, *eol;
+	char temp_buf[1024];
+	bool last_line = false;
+
 #define SYMBOL(sym) {					\
 	.str = "SYMBOL(" #sym  ")=",			\
 	.name = #sym,					\

@@ -243,7 +247,27 @@ static void scan_vmcoreinfo(char *start,
 		/* Find the end of the current line */
 		for (eol = pos; (eol <= last) && (*eol != '\n') ; eol++)
 			;
-		len = eol - pos + 1;
+		if (eol > last) {
+			/*
+			 * We did not find \n and note ended. Currently kernel
+			 * is appending last field CRASH_TIME without \n. It
+			 * is ugly but handle it.
+			 */
+			eol = last;
+			len = eol - pos + 1;
+			if (len >= sizeof(temp_buf))
+				len = sizeof(temp_buf) - 1;
+			strncpy(temp_buf, pos, len);
+			temp_buf[len + 1] = '\0';
+
+			pos = temp_buf;
+			len = len + 1;
+			eol = pos + len -1;
+			last_line = true;
+		} else  {
+			len = eol - pos + 1;
+		}
+
 		/* Stomp the last character so I am guaranteed a terminating null */
 		*eol = '\0';
 		/* Copy OSRELEASE if I see it */

@@ -266,6 +290,9 @@ static void scan_vmcoreinfo(char *start,
 			/* Remember the virtual address */
 			*symbol[i].vaddr = vaddr;
 		}
+
+		if (last_line)
+			break;
 	}
 }
 

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

On Wed, Jul 18, 2012 at 09:33:51AM -0400, Vivek Goyal wrote:

[..]

Quoted 10 lines

Quoted 7 lines

vmcore-dmesg/vmcore-dmesg.c static void scan_vmcoreinfo(char *start, · line 225

Quoted 2 lines

vmcore-dmesg/vmcore-dmesg.c +2 -0

+	char temp_buf[1024];
+	bool last_line = false;

Is there a chance of over-running temp_buf?

I would be more comfortable if there was a check below to ensure that len can never be greater than sizeof(temp_buf).

Hi Simon,

Quoted 3 lines

Agreed. Making sure temp_buf does not overflow is a good idea. Here is the udpated patch.

Hi Simon,

Do you have any more concerns with this patch. If not, can you please apply it.

Quoted 41 lines · Simon Horman

Quoted 11 lines · Vivek Goyal

scan_vmcoreinfo() currently assumes that every line in vmcoreinfo note ends with \n and overwrites new line character with \0. But last entry in note, CRASHTIME= does not end with \n and this leads to corrupting memory as we write beyond end of buffer.

Normally things were fine but when I added some fields to vmcoreinfo, this bug started showing and vmcore-dmesg started crashing.

I am planning to send a patch to fix this in kernel but it might be good idea to handle this case in user space too so that vmcore-dmesg works fine with cores of older kernels.

Good plan.

Quoted 18 lines

Signed-off-by Vivek Goyal <[email protected]>

---
 vmcore-dmesg/vmcore-dmesg.c |   27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c
===================================================================

vmcore-dmesg/vmcore-dmesg.c +1 -0

@@ -14,6 +14,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <elf.h>
+#include <stdbool.h>
 
 /* The 32bit and 64bit note headers make it clear we don't care */
 typedef Elf32_Nhdr Elf_Nhdr;

vmcore-dmesg/vmcore-dmesg.c static void scan_vmcoreinfo(char *start, · line 225

Quoted 5 lines

vmcore-dmesg/vmcore-dmesg.c +2 -0

@@ -220,6 +221,9 @@ static void scan_vmcoreinfo(char *start,
 {
 	char *last = start + size - 1;
 	char *pos, *eol;
+	char temp_buf[1024];
+	bool last_line = false;

Is there a chance of over-running temp_buf?

Quoted 2 lines

I would be more comfortable if there was a check below to ensure that len can never be greater than sizeof(temp_buf).

Hi Simon,

Agreed. Making sure temp_buf does not overflow is a good idea. Here is the udpated patch.