[PATCH] insane: detect and warn about relocations in .text

[PATCH] insane: detect and warn about relocations in .text

[Phil Blundell]

Signed-off-by: Phil Blundell <pb@pbcl.net>
---
This requires qa.elf.run_objdump() so needs to be applied after the
patch which adds that function.

 meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 5848ab4..ff35ed8 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -108,7 +108,7 @@ def package_qa_get_machine_dict():
 
 
 # Currently not being used by default "desktop"
-WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
+WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
 
 ALL_QA = "${WARN_QA} ${ERROR_QA}"
@@ -421,6 +421,30 @@ def package_qa_check_desktop(path, name, d, elf, messages):
         for l in output:
             messages.append("Desktop file issue: " + l.strip())
 
+QAPATHTEST[textrel] = "package_qa_textrel"
+def package_qa_textrel(path, name, d, elf, messages):
+    """
+    Check if the binary contains relocations in .text
+    """
+
+    if not elf:
+        return
+
+    if os.path.islink(path):
+        return
+
+    phdrs = elf.run_objdump("-p", d)
+    sane = True
+
+    import re
+    textrel_re = re.compile("\s+TEXTREL\s+")
+    for line in phdrs.split("\n"):
+        if textrel_re.match(line):
+	   sane = False
+
+    if not sane:
+        messages.append("ELF binary '%s' has relocations in .text" % path)
+
 QAPATHTEST[ldflags] = "package_qa_hash_style"
 def package_qa_hash_style(path, name, d, elf, messages):
     """
-- 
1.7.10.4

Re: [PATCH] insane: detect and warn about relocations in .text

[Martin Jansa]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> Signed-off-by: Phil Blundell <pb@pbcl.net>

Can you add a bit longer description of possible issues with relocations
in .text? So that people seeing this issue will know how dangerous it is
for them?

From my understanding (after reading
http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
performance issue?

Cheers,

> ---
> This requires qa.elf.run_objdump() so needs to be applied after the
> patch which adds that function.
> 
>  meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 5848ab4..ff35ed8 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -108,7 +108,7 @@ def package_qa_get_machine_dict():
>  
>  
>  # Currently not being used by default "desktop"
> -WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
> +WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
>  
>  ALL_QA = "${WARN_QA} ${ERROR_QA}"
> @@ -421,6 +421,30 @@ def package_qa_check_desktop(path, name, d, elf, messages):
>          for l in output:
>              messages.append("Desktop file issue: " + l.strip())
>  
> +QAPATHTEST[textrel] = "package_qa_textrel"
> +def package_qa_textrel(path, name, d, elf, messages):
> +    """
> +    Check if the binary contains relocations in .text
> +    """
> +
> +    if not elf:
> +        return
> +
> +    if os.path.islink(path):
> +        return
> +
> +    phdrs = elf.run_objdump("-p", d)
> +    sane = True
> +
> +    import re
> +    textrel_re = re.compile("\s+TEXTREL\s+")
> +    for line in phdrs.split("\n"):
> +        if textrel_re.match(line):
> +	   sane = False
> +
> +    if not sane:
> +        messages.append("ELF binary '%s' has relocations in .text" % path)
> +
>  QAPATHTEST[ldflags] = "package_qa_hash_style"
>  def package_qa_hash_style(path, name, d, elf, messages):
>      """
> -- 
> 1.7.10.4
> 
> 
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core

-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

Re: [PATCH] insane: detect and warn about relocations in .text

[Phil Blundell]

On Wed, 2012-10-03 at 12:31 +0200, Martin Jansa wrote:
> On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> > Signed-off-by: Phil Blundell <pb@pbcl.net>
> 
> Can you add a bit longer description of possible issues with relocations
> in .text? So that people seeing this issue will know how dangerous it is
> for them?
> 
> From my understanding (after reading
> http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
> performance issue?

Yes, that's correct.  It basically falls into the same sort of category
as useless-rpaths; the binary will still work, but there will be some
adverse impact on performance and memory usage.  

Historically, the most common cause of DT_TEXTREL was accidentally
linking non-PIC code into a DSO.  Recent versions of the linker will
flatly refuse to do this on at least some architectures, though, so
hopefully this problem will just go away over time.

p.

Re: [PATCH] insane: detect and warn about relocations in .text

[Richard Purdie]

On Wed, 2012-10-03 at 11:44 +0100, Phil Blundell wrote:
> On Wed, 2012-10-03 at 12:31 +0200, Martin Jansa wrote:
> > On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> > > Signed-off-by: Phil Blundell <pb@pbcl.net>
> > 
> > Can you add a bit longer description of possible issues with relocations
> > in .text? So that people seeing this issue will know how dangerous it is
> > for them?
> > 
> > From my understanding (after reading
> > http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
> > performance issue?
> 
> Yes, that's correct.  It basically falls into the same sort of category
> as useless-rpaths; the binary will still work, but there will be some
> adverse impact on performance and memory usage.  
> 
> Historically, the most common cause of DT_TEXTREL was accidentally
> linking non-PIC code into a DSO.  Recent versions of the linker will
> flatly refuse to do this on at least some architectures, though, so
> hopefully this problem will just go away over time.

Am I right in thinking this is also a marginal help to 'security' since
if the .text segment is loaded read only, it becomes slightly harder for
certain kinds of overflow attacks to work?

Cheers,

Richard

Re: [PATCH] insane: detect and warn about relocations in .text

[Phil Blundell]

On Wed, 2012-10-03 at 12:19 +0100, Richard Purdie wrote:
> Am I right in thinking this is also a marginal help to 'security' since
> if the .text segment is loaded read only, it becomes slightly harder for
> certain kinds of overflow attacks to work?

Possibly a marginal help, though (for glibc at least) the dynamic linker
will restore the original protection on .text once the relocations have
been applied, so the window of time during which you could mount an
attack based on the writeable .text region will be fairly small.  But in
principle you're right, for best security you don't want to have any
regions which are both writeable and executable.

p.

Re: [PATCH] insane: detect and warn about relocations in .text

[Phil Blundell]

On Wed, 2012-10-03 at 11:24 +0100, Phil Blundell wrote:
> Signed-off-by: Phil Blundell <pb@pbcl.net>
> ---
> This requires qa.elf.run_objdump() so needs to be applied after the
> patch which adds that function.

By the way, the background motivation for this was that the current
oe-core version of gcc seems to be somewhat broken in this respect on
MIPS; it seems to have reverted to generating an .eh_frame with a
relocation in it which results in DT_TEXTREL on the binary.  

With -fasynchronous-unwind-tables it seems to get even worse.  I don't
quite recall exactly what the failure was there but it was bad enough to
make it unusable.

I think both of these problems are regressions from the gcc-4.6 that we
were using previously.  I'm not quite sure at what point it went wrong
though.

p.

Re: [PATCH] insane: detect and warn about relocations in .text

[Mark Hatle]

As an FYI -- if you have text relocations, then the prelinker will skip that 
binary, resulting in lower startup performance.  So as a general QA check, it's 
well worth having.

--Mark

On 10/3/12 10:42 AM, Phil Blundell wrote:
> On Wed, 2012-10-03 at 11:24 +0100, Phil Blundell wrote:
>> Signed-off-by: Phil Blundell <pb@pbcl.net>
>> ---
>> This requires qa.elf.run_objdump() so needs to be applied after the
>> patch which adds that function.
>
> By the way, the background motivation for this was that the current
> oe-core version of gcc seems to be somewhat broken in this respect on
> MIPS; it seems to have reverted to generating an .eh_frame with a
> relocation in it which results in DT_TEXTREL on the binary.
>
> With -fasynchronous-unwind-tables it seems to get even worse.  I don't
> quite recall exactly what the failure was there but it was bad enough to
> make it unusable.
>
> I think both of these problems are regressions from the gcc-4.6 that we
> were using previously.  I'm not quite sure at what point it went wrong
> though.
>
> p.
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>

Re: [PATCH] insane: detect and warn about relocations in .text

[Phil Blundell]

Ping?

On Wed, 2012-10-03 at 11:24 +0100, Phil Blundell wrote:
> Signed-off-by: Phil Blundell <pb@pbcl.net>
> ---
> This requires qa.elf.run_objdump() so needs to be applied after the
> patch which adds that function.
> 
>  meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 5848ab4..ff35ed8 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -108,7 +108,7 @@ def package_qa_get_machine_dict():
>  
> 
>  # Currently not being used by default "desktop"
> -WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
> +WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
>  
>  ALL_QA = "${WARN_QA} ${ERROR_QA}"
> @@ -421,6 +421,30 @@ def package_qa_check_desktop(path, name, d, elf, messages):
>          for l in output:
>              messages.append("Desktop file issue: " + l.strip())
>  
> +QAPATHTEST[textrel] = "package_qa_textrel"
> +def package_qa_textrel(path, name, d, elf, messages):
> +    """
> +    Check if the binary contains relocations in .text
> +    """
> +
> +    if not elf:
> +        return
> +
> +    if os.path.islink(path):
> +        return
> +
> +    phdrs = elf.run_objdump("-p", d)
> +    sane = True
> +
> +    import re
> +    textrel_re = re.compile("\s+TEXTREL\s+")
> +    for line in phdrs.split("\n"):
> +        if textrel_re.match(line):
> +	   sane = False
> +
> +    if not sane:
> +        messages.append("ELF binary '%s' has relocations in .text" % path)
> +
>  QAPATHTEST[ldflags] = "package_qa_hash_style"
>  def package_qa_hash_style(path, name, d, elf, messages):
>      """

Re: [PATCH] insane: detect and warn about relocations in .text

[Mark Hatle]

On 11/6/12 6:24 PM, Phil Blundell wrote:
> Ping?

I do think we need this.  Having the textrel does prevent prelinking as well.

--Mark

> On Wed, 2012-10-03 at 11:24 +0100, Phil Blundell wrote:
>> Signed-off-by: Phil Blundell <pb@pbcl.net>
>> ---
>> This requires qa.elf.run_objdump() so needs to be applied after the
>> patch which adds that function.
>>
>>   meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
>> index 5848ab4..ff35ed8 100644
>> --- a/meta/classes/insane.bbclass
>> +++ b/meta/classes/insane.bbclass
>> @@ -108,7 +108,7 @@ def package_qa_get_machine_dict():
>>
>>
>>   # Currently not being used by default "desktop"
>> -WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
>> +WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
>>   ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
>>
>>   ALL_QA = "${WARN_QA} ${ERROR_QA}"
>> @@ -421,6 +421,30 @@ def package_qa_check_desktop(path, name, d, elf, messages):
>>           for l in output:
>>               messages.append("Desktop file issue: " + l.strip())
>>
>> +QAPATHTEST[textrel] = "package_qa_textrel"
>> +def package_qa_textrel(path, name, d, elf, messages):
>> +    """
>> +    Check if the binary contains relocations in .text
>> +    """
>> +
>> +    if not elf:
>> +        return
>> +
>> +    if os.path.islink(path):
>> +        return
>> +
>> +    phdrs = elf.run_objdump("-p", d)
>> +    sane = True
>> +
>> +    import re
>> +    textrel_re = re.compile("\s+TEXTREL\s+")
>> +    for line in phdrs.split("\n"):
>> +        if textrel_re.match(line):
>> +	   sane = False
>> +
>> +    if not sane:
>> +        messages.append("ELF binary '%s' has relocations in .text" % path)
>> +
>>   QAPATHTEST[ldflags] = "package_qa_hash_style"
>>   def package_qa_hash_style(path, name, d, elf, messages):
>>       """
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>

Re: [PATCH] insane: detect and warn about relocations in .text

[Richard Purdie]

On Wed, 2012-11-07 at 10:59 +0100, Mark Hatle wrote:
> On 11/6/12 6:24 PM, Phil Blundell wrote:
> > Ping?
> 
> I do think we need this.  Having the textrel does prevent prelinking as well.

Coming up to release I didn't really want to add more warnings and its
slipped through the cracks. I'll likely take this next week when things
quieten down a bit, ELC-E, the OE-GA, Yocto Project Dev day and so on
are making this one a little hectic.

Does anyone know how many warnings this is going to generate on an
OE-Core world build?

The biggest issue I have with these new tests is people also stepping up
and fixing the warnings they generate since often its a non-trivial
effort. The Yocto Project has been doing a lot here, quietly fixing a
lot of them up but more help would be much appreciated.

Cheers,

Richard