[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLpZ=2GDK4LRJJwvpz-52Ftx75wsJJGoXsWysWooZv68A@mail.gmail.com>
Date: Thu, 30 Mar 2017 09:24:14 -0700
From: Kees Cook <keescook@...omium.org>
To: Jessica Yu <jeyu@...hat.com>
Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>, Catalin Marinas <catalin.marinas@....com>,
Heiko Carstens <heiko.carstens@...ibm.com>, kbuild-all@...org,
kbuild test robot <lkp@...el.com>, Rusty Russell <rusty@...tcorp.com.au>,
LKML <linux-kernel@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v4 2/2] extable: verify address is read-only
On Wed, Mar 29, 2017 at 8:27 PM, Jessica Yu <jeyu@...hat.com> wrote:
> +++ Eddie Kovsky [28/03/17 21:28 -0600]:
>
>> On 03/27/17, Kees Cook wrote:
>>>
>>> On Mon, Mar 27, 2017 at 1:43 AM, kbuild test robot <lkp@...el.com> wrote:
>>> > Hi Eddie,
>>> >
>>> > [auto build test ERROR on next-20170323]
>>> > [cannot apply to linus/master linux/master jeyu/modules-next v4.9-rc8
>>> > v4.9-rc7 v4.9-rc6 v4.11-rc4]
>>> > [if your patch is applied to the wrong git tree, please drop us a note
>>> > to help improve the system]
>>> >
>>> > url:
>>> > https://github.com/0day-ci/linux/commits/Eddie-Kovsky/module-verify-address-is-read-only/20170327-142922
>>> > config: blackfin-BF561-EZKIT-SMP_defconfig (attached as .config)
>>> > compiler: bfin-uclinux-gcc (GCC) 6.2.0
>>> > reproduce:
>>> > wget
>>> > https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
>>> > ~/bin/make.cross
>>> > chmod +x ~/bin/make.cross
>>> > # save the attached .config to linux build tree
>>> > make.cross ARCH=blackfin
>>> >
>>> > All errors (new ones prefixed by >>):
>>> >
>>> > kernel/built-in.o: In function `core_kernel_rodata':
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__start_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>> >>> kernel/extable.c:169: undefined reference to
>>> >>> `__end_data_ro_after_init'
>>>
>>> Hm, I'm confused about this. blackfin includes
>>> include/asm-generic-vmlinux.lds.h and uses the RO_DATA macro (which
>>> resolves to RO_DATA_SECTION to RO_AFTER_INIT_DATA which defines
>>> __[start|end]_data_ro_after_init.
>>>
>>> Also, it seems that commit d7c19b066dcf4bd19c4385e8065558d4e74f9e73
>>> ("mm: kmemleak: scan .data.ro_after_init") added a potentially
>>> redundant section name (s390 already calls this
>>> __[start|end]_ro_after_init). I'd like to get this cleaned up, since
>>> having multiple names for the same thing is confusing:
>>>
>>> diff --git a/arch/s390/kernel/vmlinux.lds.S
>>> b/arch/s390/kernel/vmlinux.lds.S
>>> index 000e6e91f6a0..3667d20e997f 100644
>>> --- a/arch/s390/kernel/vmlinux.lds.S
>>> +++ b/arch/s390/kernel/vmlinux.lds.S
>>> @@ -62,9 +62,11 @@ SECTIONS
>>>
>>> . = ALIGN(PAGE_SIZE);
>>> __start_ro_after_init = .;
>>> + __start_data_ro_after_init = .;
>>> .data..ro_after_init : {
>>> *(.data..ro_after_init)
>>> }
>>> + __end_data_ro_after_init = .;
>>> EXCEPTION_TABLE(16)
>>> . = ALIGN(PAGE_SIZE);
>>> __end_ro_after_init = .;
>>>
>>> And it seems that this hunk is wrong (__end_ro_after_init includes
>>> s390's exception table, etc). I think we should remove the
>>> ..._data_... name and use s390's name.
>>>
>>> I'll send an adjustment patch, but we'll still need to deal with
>>> blackfin.
>>>
>>> -Kees
>>>
>>
>> Kees
>>
>> I applied your patch (mm: fix section name for .data..ro_after_init) and
>> changed the new function in extable.c to use __[start|end]_ro_after_init
>> instead. The new version still builds without errors on x86, which isn't
>> surprising.
>>
>> I've cross compiled this for blackfin and I'm able to reproduce the
>> build error. I'm still not sure why. As you pointed out, blackfin does
>> appear to use 'include/asm-generic/vmlinux.lds.h'.
>
>
> This appears to be because blackfin is one of the 2 arches that
> prepends an underscore '_' to all symbols defined in C. I noticed that
> __{start,end}_data_ro_after_init in vmlinux.lds.h are not wrapped with
> VMLINUX_SYMBOL() which adds the necessary prefix for arches that have
> HAVE_UNDERSCORE_SYMBOL_PREFIX, hence the undefined reference.
Argh. Thank you for catching this! Yeah, that would have taken me
forever to find.
> The below patch fixed the build error for me, if it works for you then
> I can send a formal patch.
>
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 4e09b28..7b262f7 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -260,9 +260,9 @@
> */
> #ifndef RO_AFTER_INIT_DATA
> #define RO_AFTER_INIT_DATA \
> - __start_data_ro_after_init = .; \
> + VMLINUX_SYMBOL(__start_data_ro_after_init) = .; \
> *(.data..ro_after_init) \
> - __end_data_ro_after_init = .;
> + VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
> #endif
I don't have a blackfin cross-compiler set up, but I'm sure that'll
fix it. If you can, please base it on -next, since I rename
__[start|end]_data_ro_after_init to __[start|end]_ro_after_init (to
match the existing s390 symbols of the same purpose):
https://lkml.org/lkml/2017/3/27/685
akpm is carrying that patch, so this follow-up should likely go to him too.
Thanks!
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
