| Message ID | 1479980960-11046-1-git-send-email-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On November 24, 2016 4:49:20 AM EST, Ross Lagerwall <ross.lagerwall@citrix.com> wrote: >GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >means that .rodata.str* sections are now split by function. We could >probably be smarter about including just the sections we need, but for >now, include all .rodata.str* sections as is done for previous versions >of GCC. > Here you say .str* But the code only does this for .str1.* Did you mean to make it.more generic for say .rodata.*.str[0-9].* ? >This manifests itself as symbol error. E.g.: >(XEN) Unknown symbol: .LC0 > >Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >Reported-by: M A Young <m.a.young@durham.ac.uk> >--- > create-diff-object.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > >diff --git a/create-diff-object.c b/create-diff-object.c >index 69bcd88..b0d1348 100644 >--- a/create-diff-object.c >+++ b/create-diff-object.c >@@ -1184,6 +1184,43 @@ static void >kpatch_process_special_sections(struct kpatch_elf *kelf) > } > } > >+/* Returns true if s is a string of only numbers with length > 0. */ >+static int isnumber(const char *s) >+{ >+ do { >+ if (!*s || !isdigit(*s)) >+ return 0; >+ } while (*++s); >+ >+ return 1; >+} >+ >+/* >+ * String sections are always included even if unchanged. >+ * The format is either: >+ * .rodata.<func>.str1.[0-9]+ (new in GCC 6.1.0) >+ * or .rodata.str1.[0-9]+ (older versions of GCC) >+ * For the new format we could be smarter and only include the needed >+ * strings sections. >+ */ >+static int should_include_str_section(const char *name) >+{ >+ const char *s; >+ >+ if (strncmp(name, ".rodata.", 8)) >+ return 0; >+ >+ /* Check if name matches ".rodata.str1.[0-9]+" */ >+ if (!strncmp(name, ".rodata.str1.", 13)) >+ return isnumber(name + 13); >+ >+ /* Check if name matches ".rodata.<func>.str1.[0-9]+" */ >+ s = strstr(name, ".str1."); >+ if (!s) >+ return 0; >+ return isnumber(s + 6); >+} >+ > static void kpatch_include_standard_elements(struct kpatch_elf *kelf) > { > struct section *sec; >@@ -1193,7 +1230,7 @@ static void >kpatch_include_standard_elements(struct kpatch_elf *kelf) > if (!strcmp(sec->name, ".shstrtab") || > !strcmp(sec->name, ".strtab") || > !strcmp(sec->name, ".symtab") || >- !strncmp(sec->name, ".rodata.str1.", 13)) { >+ should_include_str_section(sec->name)) { > sec->include = 1; > if (sec->secsym) > sec->secsym->include = 1; Thanks!
On 11/24/2016 12:31 PM, Konrad Rzeszutek Wilk wrote: > On November 24, 2016 4:49:20 AM EST, Ross Lagerwall <ross.lagerwall@citrix.com> wrote: >> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >> means that .rodata.str* sections are now split by function. We could >> probably be smarter about including just the sections we need, but for >> now, include all .rodata.str* sections as is done for previous versions >> of GCC. >> > > Here you say .str* > > But the code only does this for .str1.* > > Did you mean to make it.more generic for say .rodata.*.str[0-9].* > > ? > The code is what I intended. I tweaked the commit message slightly: GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which means that .rodata.str1.[0-9]+ sections are now split by function. We could probably be smarter about including just the sections we need, but for now, simply include the string sections for all functions as is done for previous versions of GCC.
On Thu, 24 Nov 2016, Ross Lagerwall wrote: > GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which > means that .rodata.str* sections are now split by function. We could > probably be smarter about including just the sections we need, but for > now, include all .rodata.str* sections as is done for previous versions > of GCC. > > This manifests itself as symbol error. E.g.: > (XEN) Unknown symbol: .LC0 There may be a problem with this patch. I built livepatch-build-tools (from the xenbits git repo) with this and the other patch posted yesterday and successfully built and applied xsa191 to xsa193 (cumulatively) to xen-4.8.0-rc6, but the computer freezes if I try to apply xsa194 (cumulatively or on its own). This was the only patch I tried which got the Unknown symbol: .LC3 message ie. (XEN) livepatch_elf.c:295: livepatch: xsa194: Unknown symbol: .LC3 so this may be related to the crash. Michael Young
On 25/11/16 16:59, M A Young wrote: > On Thu, 24 Nov 2016, Ross Lagerwall wrote: > >> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >> means that .rodata.str* sections are now split by function. We could >> probably be smarter about including just the sections we need, but for >> now, include all .rodata.str* sections as is done for previous versions >> of GCC. >> >> This manifests itself as symbol error. E.g.: >> (XEN) Unknown symbol: .LC0 > There may be a problem with this patch. I built livepatch-build-tools > (from the xenbits git repo) with this and the other patch posted yesterday > and successfully built and applied xsa191 to xsa193 (cumulatively) to > xen-4.8.0-rc6, but the computer freezes if I try to apply xsa194 > (cumulatively or on its own). This was the only patch I tried which got > the Unknown symbol: .LC3 message ie. > (XEN) livepatch_elf.c:295: livepatch: xsa194: Unknown symbol: .LC3 > so this may be related to the crash. XSA-194 is a toolstack patch. It isn't applicable to livepatch. There is one copy of the vulnerable code in Xen, but it is only used to construct dom0 and discarded along with all the other __init code. Such a livepatch should be rejected by Xen... ~Andrew
On Fri, Nov 25, 2016 at 04:59:17PM +0000, M A Young wrote: > On Thu, 24 Nov 2016, Ross Lagerwall wrote: > > > GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which > > means that .rodata.str* sections are now split by function. We could > > probably be smarter about including just the sections we need, but for > > now, include all .rodata.str* sections as is done for previous versions > > of GCC. > > > > This manifests itself as symbol error. E.g.: > > (XEN) Unknown symbol: .LC0 > > There may be a problem with this patch. I built livepatch-build-tools > (from the xenbits git repo) with this and the other patch posted yesterday > and successfully built and applied xsa191 to xsa193 (cumulatively) to > xen-4.8.0-rc6, but the computer freezes if I try to apply xsa194 > (cumulatively or on its own). This was the only patch I tried which got > the Unknown symbol: .LC3 message ie. > (XEN) livepatch_elf.c:295: livepatch: xsa194: Unknown symbol: .LC3 > so this may be related to the crash. Anything on the serial console? Can you use the crash functionality? Pls make sure to boot with 'loglvl=all'. Also you may want to build it with 'debug=y' so the livepatch debug errors show up too. Thanks! > > Michael Young
On 11/25/2016 05:05 PM, Andrew Cooper wrote: > On 25/11/16 16:59, M A Young wrote: >> On Thu, 24 Nov 2016, Ross Lagerwall wrote: >> >>> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >>> means that .rodata.str* sections are now split by function. We could >>> probably be smarter about including just the sections we need, but for >>> now, include all .rodata.str* sections as is done for previous versions >>> of GCC. >>> >>> This manifests itself as symbol error. E.g.: >>> (XEN) Unknown symbol: .LC0 >> There may be a problem with this patch. I built livepatch-build-tools >> (from the xenbits git repo) with this and the other patch posted yesterday >> and successfully built and applied xsa191 to xsa193 (cumulatively) to >> xen-4.8.0-rc6, but the computer freezes if I try to apply xsa194 >> (cumulatively or on its own). This was the only patch I tried which got >> the Unknown symbol: .LC3 message ie. >> (XEN) livepatch_elf.c:295: livepatch: xsa194: Unknown symbol: .LC3 >> so this may be related to the crash. > > XSA-194 is a toolstack patch. It isn't applicable to livepatch. > > There is one copy of the vulnerable code in Xen, but it is only used to > construct dom0 and discarded along with all the other __init code. > > Such a livepatch should be rejected by Xen... > elf_init() is not marked __init, so it is included in the live patch.
On 25/11/16 17:16, Ross Lagerwall wrote: > On 11/25/2016 05:05 PM, Andrew Cooper wrote: >> On 25/11/16 16:59, M A Young wrote: >>> On Thu, 24 Nov 2016, Ross Lagerwall wrote: >>> >>>> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which >>>> means that .rodata.str* sections are now split by function. We could >>>> probably be smarter about including just the sections we need, but for >>>> now, include all .rodata.str* sections as is done for previous >>>> versions >>>> of GCC. >>>> >>>> This manifests itself as symbol error. E.g.: >>>> (XEN) Unknown symbol: .LC0 >>> There may be a problem with this patch. I built livepatch-build-tools >>> (from the xenbits git repo) with this and the other patch posted >>> yesterday >>> and successfully built and applied xsa191 to xsa193 (cumulatively) to >>> xen-4.8.0-rc6, but the computer freezes if I try to apply xsa194 >>> (cumulatively or on its own). This was the only patch I tried which got >>> the Unknown symbol: .LC3 message ie. >>> (XEN) livepatch_elf.c:295: livepatch: xsa194: Unknown symbol: .LC3 >>> so this may be related to the crash. >> >> XSA-194 is a toolstack patch. It isn't applicable to livepatch. >> >> There is one copy of the vulnerable code in Xen, but it is only used to >> construct dom0 and discarded along with all the other __init code. >> >> Such a livepatch should be rejected by Xen... >> > > elf_init() is not marked __init, so it is included in the live patch. Well that's unfortunate, as it does genuinely live in .init. The entirety of libelf is objcpy'd libelf.o: libelf-temp.o Makefile $(OBJCOPY) $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ ~Andrew
diff --git a/create-diff-object.c b/create-diff-object.c index 69bcd88..b0d1348 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -1184,6 +1184,43 @@ static void kpatch_process_special_sections(struct kpatch_elf *kelf) } } +/* Returns true if s is a string of only numbers with length > 0. */ +static int isnumber(const char *s) +{ + do { + if (!*s || !isdigit(*s)) + return 0; + } while (*++s); + + return 1; +} + +/* + * String sections are always included even if unchanged. + * The format is either: + * .rodata.<func>.str1.[0-9]+ (new in GCC 6.1.0) + * or .rodata.str1.[0-9]+ (older versions of GCC) + * For the new format we could be smarter and only include the needed + * strings sections. + */ +static int should_include_str_section(const char *name) +{ + const char *s; + + if (strncmp(name, ".rodata.", 8)) + return 0; + + /* Check if name matches ".rodata.str1.[0-9]+" */ + if (!strncmp(name, ".rodata.str1.", 13)) + return isnumber(name + 13); + + /* Check if name matches ".rodata.<func>.str1.[0-9]+" */ + s = strstr(name, ".str1."); + if (!s) + return 0; + return isnumber(s + 6); +} + static void kpatch_include_standard_elements(struct kpatch_elf *kelf) { struct section *sec; @@ -1193,7 +1230,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf) if (!strcmp(sec->name, ".shstrtab") || !strcmp(sec->name, ".strtab") || !strcmp(sec->name, ".symtab") || - !strncmp(sec->name, ".rodata.str1.", 13)) { + should_include_str_section(sec->name)) { sec->include = 1; if (sec->secsym) sec->secsym->include = 1;
GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which means that .rodata.str* sections are now split by function. We could probably be smarter about including just the sections we need, but for now, include all .rodata.str* sections as is done for previous versions of GCC. This manifests itself as symbol error. E.g.: (XEN) Unknown symbol: .LC0 Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Reported-by: M A Young <m.a.young@durham.ac.uk> --- create-diff-object.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)