* Re: [PATCH] insane: detect and warn about relocations in .text
@ 2012-11-06 17:24 Phil Blundell
2012-11-07 9:59 ` Mark Hatle
0 siblings, 1 reply; 10+ messages in thread
From: Phil Blundell @ 2012-11-06 17:24 UTC (permalink / raw)
To: oe-core
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):
> """
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] insane: detect and warn about relocations in .text
2012-11-06 17:24 [PATCH] insane: detect and warn about relocations in .text Phil Blundell
@ 2012-11-07 9:59 ` Mark Hatle
2012-11-07 14:37 ` Richard Purdie
0 siblings, 1 reply; 10+ messages in thread
From: Mark Hatle @ 2012-11-07 9:59 UTC (permalink / raw)
To: openembedded-core
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] insane: detect and warn about relocations in .text
2012-11-07 9:59 ` Mark Hatle
@ 2012-11-07 14:37 ` Richard Purdie
0 siblings, 0 replies; 10+ messages in thread
From: Richard Purdie @ 2012-11-07 14:37 UTC (permalink / raw)
To: Mark Hatle; +Cc: openembedded-core
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] insane: detect and warn about relocations in .text
@ 2012-10-03 15:42 Phil Blundell
2012-10-03 15:56 ` Mark Hatle
0 siblings, 1 reply; 10+ messages in thread
From: Phil Blundell @ 2012-10-03 15:42 UTC (permalink / raw)
To: oe-core
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] insane: detect and warn about relocations in .text
2012-10-03 15:42 Phil Blundell
@ 2012-10-03 15:56 ` Mark Hatle
0 siblings, 0 replies; 10+ messages in thread
From: Mark Hatle @ 2012-10-03 15:56 UTC (permalink / raw)
To: openembedded-core
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] insane: detect and warn about relocations in .text
@ 2012-10-03 10:24 Phil Blundell
2012-10-03 10:31 ` Martin Jansa
0 siblings, 1 reply; 10+ messages in thread
From: Phil Blundell @ 2012-10-03 10:24 UTC (permalink / raw)
To: oe-core
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] insane: detect and warn about relocations in .text
2012-10-03 10:24 Phil Blundell
@ 2012-10-03 10:31 ` Martin Jansa
2012-10-03 10:44 ` Phil Blundell
0 siblings, 1 reply; 10+ messages in thread
From: Martin Jansa @ 2012-10-03 10:31 UTC (permalink / raw)
To: Phil Blundell; +Cc: oe-core
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] insane: detect and warn about relocations in .text
2012-10-03 10:31 ` Martin Jansa
@ 2012-10-03 10:44 ` Phil Blundell
2012-10-03 11:19 ` Richard Purdie
0 siblings, 1 reply; 10+ messages in thread
From: Phil Blundell @ 2012-10-03 10:44 UTC (permalink / raw)
To: Martin Jansa; +Cc: oe-core
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] insane: detect and warn about relocations in .text
2012-10-03 10:44 ` Phil Blundell
@ 2012-10-03 11:19 ` Richard Purdie
2012-10-03 12:39 ` Phil Blundell
0 siblings, 1 reply; 10+ messages in thread
From: Richard Purdie @ 2012-10-03 11:19 UTC (permalink / raw)
To: Phil Blundell; +Cc: Martin Jansa, oe-core
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] insane: detect and warn about relocations in .text
2012-10-03 11:19 ` Richard Purdie
@ 2012-10-03 12:39 ` Phil Blundell
0 siblings, 0 replies; 10+ messages in thread
From: Phil Blundell @ 2012-10-03 12:39 UTC (permalink / raw)
To: Richard Purdie; +Cc: Martin Jansa, oe-core
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-07 14:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-06 17:24 [PATCH] insane: detect and warn about relocations in .text Phil Blundell
2012-11-07 9:59 ` Mark Hatle
2012-11-07 14:37 ` Richard Purdie
-- strict thread matches above, loose matches on Subject: below --
2012-10-03 15:42 Phil Blundell
2012-10-03 15:56 ` Mark Hatle
2012-10-03 10:24 Phil Blundell
2012-10-03 10:31 ` Martin Jansa
2012-10-03 10:44 ` Phil Blundell
2012-10-03 11:19 ` Richard Purdie
2012-10-03 12:39 ` Phil Blundell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox