On Tue, Mar 17, 2015 at 01:40:02PM +0100, Quentin Casasnovas wrote:
> __ex_table is a simple table section where each entry is a pair of
> addresses - the first address is an address which can fault in kernel
> space, and the second address points to where the kernel should jump to
> when handling that fault. This is how copy_from_user() does not crash the
> kernel if userspace gives a borked pointer for example.
>
> If one of these addresses point to a non-executable section, something is
> seriously wrong since it either means the kernel will never fault from
> there or it will not be able to jump to there. As both cases are serious
> enough, we simply error out in these cases so the build fails and the
> developper has to fix the issue.
>
> In case the section is executable, but it isn't referenced in our list of
> authorized sections to point to from __ex_table, we just dump a warning
> giving more information about it. We do this in case the new section is
> executable but isn't supposed to be executed by the kernel. This happened
> with .altinstr_replacement, which is executable but is only used to copy
> instructions from - we should never have our instruction pointer pointing
> in .altinstr_replacement. Admitedly, a proper fix in that case would be to
> just set .altinstr_replacement NX, but we need to warn about future cases
> like this.
>
> Signed-off-by: Quentin Casasnovas <[email protected]>
> ---
> scripts/mod/modpost.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
This causes a bunch of mismatch warnings on 32-bit and 64-bit ARM
because there are two additional sections, .text.fixup and
.exception.text that store executable code. I've attached a patch
to fix those, but feel free to squash that into the original commit
if that's still possible.
Also adding Rusty since he applied this to the modules-next tree.
Thierry
On Tue, Apr 14, 2015 at 02:14:14PM +0200, Thierry Reding wrote:
> On Tue, Mar 17, 2015 at 01:40:02PM +0100, Quentin Casasnovas wrote:
> > If one of these addresses point to a non-executable section, something is
> > seriously wrong since it either means the kernel will never fault from
> > there or it will not be able to jump to there. As both cases are serious
> > enough, we simply error out in these cases so the build fails and the
> > developper has to fix the issue.
> >
> > Signed-off-by: Quentin Casasnovas <[email protected]>
> > ---
> > scripts/mod/modpost.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 141 insertions(+)
>
> This causes a bunch of mismatch warnings on 32-bit and 64-bit ARM
> because there are two additional sections, .text.fixup and
> .exception.text that store executable code. I've attached a patch
> to fix those, but feel free to squash that into the original commit
> if that's still possible.
>
Thanks Thierry!
Your patch looks good to me, though I was wondering if we should just add
.text.* in the TEXT_SECTIONS macro. Some architectures define
-ffunction-sections (parisc, score, metag and frv) so there are tons of
useless warnings on these.. It also means the current modpost sanity
checks don't run for those so it might even uncover some real mismatch ;)
Quentin
Quentin Casasnovas <[email protected]> writes:
> On Tue, Apr 14, 2015 at 02:14:14PM +0200, Thierry Reding wrote:
>> On Tue, Mar 17, 2015 at 01:40:02PM +0100, Quentin Casasnovas wrote:
>> > If one of these addresses point to a non-executable section, something is
>> > seriously wrong since it either means the kernel will never fault from
>> > there or it will not be able to jump to there. As both cases are serious
>> > enough, we simply error out in these cases so the build fails and the
>> > developper has to fix the issue.
>> >
>> > Signed-off-by: Quentin Casasnovas <[email protected]>
>> > ---
>> > scripts/mod/modpost.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 141 insertions(+)
>>
>> This causes a bunch of mismatch warnings on 32-bit and 64-bit ARM
>> because there are two additional sections, .text.fixup and
>> .exception.text that store executable code. I've attached a patch
>> to fix those, but feel free to squash that into the original commit
>> if that's still possible.
>>
>
> Thanks Thierry!
>
> Your patch looks good to me, though I was wondering if we should just add
> .text.* in the TEXT_SECTIONS macro. Some architectures define
> -ffunction-sections (parisc, score, metag and frv) so there are tons of
> useless warnings on these.. It also means the current modpost sanity
> checks don't run for those so it might even uncover some real mismatch ;)
Yes, but this adds ".exception.text" so a .text.* wildcard won't quite
cover it.
I've applied his patch, then the following:
modpost: handle -ffunction-sections
52dc0595d540 introduced OTHER_TEXT_SECTIONS for identifying what
sections could validly have __ex_table entries. Unfortunately, it
wasn't tested with -ffunction-sections, which some architectures
use.
Reported-by: kbuild test robot <[email protected]>
Cc: Quentin Casasnovas <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index cbd53e08769d..22dbc604cdb9 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -876,7 +876,7 @@ static void check_section(const char *modname, struct elf_info *elf,
#define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \
".kprobes.text"
#define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
- ".fixup", ".entry.text"
+ ".fixup", ".entry.text", ".exception.text", ".text.*"
#define INIT_SECTIONS ".init.*"
#define MEM_INIT_SECTIONS ".meminit.*"
On Wed, Apr 15, 2015 at 12:57:37PM +0930, Rusty Russell wrote:
>
> I've applied his patch, then the following:
Thanks.
>
> modpost: handle -ffunction-sections
>
> 52dc0595d540 introduced OTHER_TEXT_SECTIONS for identifying what
> sections could validly have __ex_table entries. Unfortunately, it
> wasn't tested with -ffunction-sections, which some architectures
> use.
>
> Reported-by: kbuild test robot <[email protected]>
> Cc: Quentin Casasnovas <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index cbd53e08769d..22dbc604cdb9 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -876,7 +876,7 @@ static void check_section(const char *modname, struct elf_info *elf,
> #define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \
> ".kprobes.text"
> #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \
> - ".fixup", ".entry.text"
> + ".fixup", ".entry.text", ".exception.text", ".text.*"
>
Is there a reason we're not adding ".text.*" to TEXT_SECTIONS as opposed to
OTHER_TEXT_SECTIONS? AFAIU, we'll not run the other modpost mismatch
checks when the kernel is compiled with -ffunction-sections otherwise.
I'll send a tentative fix for the divide-by-zero error shortly, sorry about
the mess.
Quentin