[PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Ross Lagerwall]

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(-)

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;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Konrad Rzeszutek Wilk]

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!


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Ross Lagerwall]

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.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[M A Young]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Konrad Rzeszutek Wilk]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Ross Lagerwall]

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.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Andrew Cooper]

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel