* [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer
@ 2012-07-17 15:23 Vivek Goyal
2012-07-17 22:40 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2012-07-17 15:23 UTC (permalink / raw)
To: Kexec Mailing List, horms; +Cc: Eric W. Biederman
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 <vgoyal@redhat.com>
---
vmcore-dmesg/vmcore-dmesg.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c
===================================================================
--- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:54:02.700700235 -0400
+++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:55:08.232702248 -0400
@@ -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,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 */
@@ -266,6 +288,9 @@ static void scan_vmcoreinfo(char *start,
/* Remember the virtual address */
*symbol[i].vaddr = vaddr;
}
+
+ if (last_line)
+ break;
}
}
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer 2012-07-17 15:23 [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer Vivek Goyal @ 2012-07-17 22:40 ` Simon Horman 2012-07-18 13:33 ` Vivek Goyal 0 siblings, 1 reply; 6+ messages in thread From: Simon Horman @ 2012-07-17 22:40 UTC (permalink / raw) To: Vivek Goyal; +Cc: Kexec Mailing List, Eric W. Biederman On Tue, Jul 17, 2012 at 11:23:00AM -0400, Vivek Goyal wrote: > 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. > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > vmcore-dmesg/vmcore-dmesg.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c > =================================================================== > --- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:54:02.700700235 -0400 > +++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:55:08.232702248 -0400 > @@ -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; 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). > + > #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 */ > @@ -266,6 +288,9 @@ static void scan_vmcoreinfo(char *start, > /* Remember the virtual address */ > *symbol[i].vaddr = vaddr; > } > + > + if (last_line) > + break; > } > } > > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer 2012-07-17 22:40 ` Simon Horman @ 2012-07-18 13:33 ` Vivek Goyal 2012-07-23 14:35 ` Vivek Goyal 2012-07-24 0:34 ` Simon Horman 0 siblings, 2 replies; 6+ messages in thread From: Vivek Goyal @ 2012-07-18 13:33 UTC (permalink / raw) To: Simon Horman; +Cc: Kexec Mailing List, Eric W. Biederman On Wed, Jul 18, 2012 at 07:40:32AM +0900, Simon Horman wrote: > On Tue, Jul 17, 2012 at 11:23:00AM -0400, Vivek Goyal wrote: > > 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. > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > vmcore-dmesg/vmcore-dmesg.c | 27 ++++++++++++++++++++++++++- > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c > > =================================================================== > > --- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:54:02.700700235 -0400 > > +++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:55:08.232702248 -0400 > > @@ -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; > > 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, 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 <vgoyal@redhat.com> --- vmcore-dmesg/vmcore-dmesg.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c =================================================================== --- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-20 08:04:42.370051049 -0400 +++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-20 08:12:14.198070252 -0400 @@ -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 kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer 2012-07-18 13:33 ` Vivek Goyal @ 2012-07-23 14:35 ` Vivek Goyal 2012-07-23 23:08 ` Simon Horman 2012-07-24 0:34 ` Simon Horman 1 sibling, 1 reply; 6+ messages in thread From: Vivek Goyal @ 2012-07-23 14:35 UTC (permalink / raw) To: Simon Horman; +Cc: Kexec Mailing List, Eric W. Biederman On Wed, Jul 18, 2012 at 09:33:51AM -0400, Vivek Goyal wrote: [..] > > > + 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, > > 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. Thanks Vivek > 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 <vgoyal@redhat.com> > --- > vmcore-dmesg/vmcore-dmesg.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c > =================================================================== > --- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-20 08:04:42.370051049 -0400 > +++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-20 08:12:14.198070252 -0400 > @@ -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 > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer 2012-07-23 14:35 ` Vivek Goyal @ 2012-07-23 23:08 ` Simon Horman 0 siblings, 0 replies; 6+ messages in thread From: Simon Horman @ 2012-07-23 23:08 UTC (permalink / raw) To: Vivek Goyal; +Cc: Kexec Mailing List, Eric W. Biederman On Mon, Jul 23, 2012 at 10:35:12AM -0400, Vivek Goyal wrote: > On Wed, Jul 18, 2012 at 09:33:51AM -0400, Vivek Goyal wrote: > > [..] > > > > + 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, > > > > 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. Sorry, I missed the updated patch somehow. I'll apply it ASAP. _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer 2012-07-18 13:33 ` Vivek Goyal 2012-07-23 14:35 ` Vivek Goyal @ 2012-07-24 0:34 ` Simon Horman 1 sibling, 0 replies; 6+ messages in thread From: Simon Horman @ 2012-07-24 0:34 UTC (permalink / raw) To: Vivek Goyal; +Cc: Kexec Mailing List, Eric W. Biederman On Wed, Jul 18, 2012 at 09:33:51AM -0400, Vivek Goyal wrote: > On Wed, Jul 18, 2012 at 07:40:32AM +0900, Simon Horman wrote: > > On Tue, Jul 17, 2012 at 11:23:00AM -0400, Vivek Goyal wrote: > > > 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. > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > --- > > > vmcore-dmesg/vmcore-dmesg.c | 27 ++++++++++++++++++++++++++- > > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > > > Index: kexec-tools/vmcore-dmesg/vmcore-dmesg.c > > > =================================================================== > > > --- kexec-tools.orig/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:54:02.700700235 -0400 > > > +++ kexec-tools/vmcore-dmesg/vmcore-dmesg.c 2012-07-19 01:55:08.232702248 -0400 > > > @@ -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; > > > > 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, > > Agreed. Making sure temp_buf does not overflow is a good idea. Here is > the udpated patch. Thanks, applied. _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-24 0:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-17 15:23 [PATCH] vmcore-dmesg: Do not write beyond end of allocated buffer Vivek Goyal 2012-07-17 22:40 ` Simon Horman 2012-07-18 13:33 ` Vivek Goyal 2012-07-23 14:35 ` Vivek Goyal 2012-07-23 23:08 ` Simon Horman 2012-07-24 0:34 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).