| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0c122390-6e76-f773-86e9-8c085f4384f2@nokia.com>
Date: Mon, 8 Mar 2021 08:58:29 +0100
From: Alexander Sverdlin <alexander.sverdlin@...ia.com>
To: Qais Yousef <qais.yousef@....com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Russell King <linux@...linux.org.uk>,
linux-arm-kernel@...ts.infradead.org,
Florian Fainelli <f.fainelli@...il.com>,
linux-kernel@...r.kernel.org,
Ard Biesheuvel <ard.biesheuvel@...aro.org>
Subject: Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hi!
On 07/03/2021 18:26, Qais Yousef wrote:
> I tried on 5.12-rc2 and 5.11 but couldn't reproduce the problem using your
> instructions on the other email. But most likely because I'm hitting another
> problem that could be masking it. I'm not sure it is related or just randomly
> happened to hit it.
>
> Did you see something similar?
[...]
> [ 0.000000] [<c1b01a38>] (ftrace_bug) from [<c046316c>] (ftrace_process_locs+0x2b0/0x518)
> [ 0.000000] r7:c3817ac4 r6:c38040c0 r5:00000a3c r4:000134e4
> [ 0.000000] [<c0462ebc>] (ftrace_process_locs) from [<c2b25240>] (ftrace_init+0xc8/0x174)
> [ 0.000000] r10:c2ffa000 r9:c2be8a78 r8:c2c5d1fc r7:c2c0c208 r6:00000001 r5:c2d0908c
> [ 0.000000] r4:c362f518
> [ 0.000000] [<c2b25178>] (ftrace_init) from [<c2b00e14>] (start_kernel+0x2f4/0x5b8)
> [ 0.000000] r9:c2be8a78 r8:dbfffec0 r7:00000000 r6:c36385cc r5:c2d08f00 r4:c2ffa000
> [ 0.000000] [<c2b00b20>] (start_kernel) from [<00000000>] (0x0)
This means, FTRACE has more problems with your kernel/compiler/platform, I've addressed similar issue
in the past, but my patch should be long merged:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1817963.html
Could it be the same problem as here:
https://www.spinics.net/lists/arm-kernel/msg854022.html
Seems that the size check deserves something line BUILD_BUG_ON() with FTRACE...
>> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
>> index 9a79ef6..fa867a5 100644
>> --- a/arch/arm/kernel/ftrace.c
>> +++ b/arch/arm/kernel/ftrace.c
>> @@ -70,6 +70,19 @@ int ftrace_arch_code_modify_post_process(void)
>>
>> static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
>> {
>> + s32 offset = addr - pc;
>> + s32 blim = 0xfe000008;
>> + s32 flim = 0x02000004;
>
> This look like magic numbers to me..
These magic numbers are most probably the reason for your FTRACE to resign...
Those are backward- and forward-branch limits. I didn't find the matching DEFINEs
in the kernel, but I would be happy to learn them. I can also put some comments,
but I actually thought the purpose would be obvious from the code...
>> +
>> + if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
>> + blim = 0xff000004;
>> + flim = 0x01000002;
>
> .. ditto ..
>
>> + }
>> +
>> + if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) &&
>> + (offset < blim || offset > flim))
>> + return 0;
>
> .. I could have missed something, but wouldn't something like below be clearer?
> Only compile tested. I think abs() will do the right thing here given the
> passed types. I admit I don't understand why you have the '4' and '8' at the
> lowest nibble..
Yes, the limits are not symmetrical. These "magic numbers" have been checked many
times by me, but I admit I'm not expert in ARM assembly. I'm however still quite
sure about them.
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index fa867a57100f..b44aee87c53a 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -70,17 +70,13 @@ int ftrace_arch_code_modify_post_process(void)
>
> static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
> {
> - s32 offset = addr - pc;
> - s32 blim = 0xfe000008;
> - s32 flim = 0x02000004;
> + u32 offset = abs(addr - pc);
> + u32 range = 0x02000000; /* +-32MiB */
>
> - if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
> - blim = 0xff000004;
> - flim = 0x01000002;
> - }
> + if (IS_ENABLED(CONFIG_THUMB2_KERNEL))
> + range = 0x01000000; /* +-16MiB */
>
> - if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) &&
> - (offset < blim || offset > flim))
> + if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) && offset > range)
> return 0;
See above, the limits are not symmetrical.
> return arm_gen_branch_link(pc, addr);
>
> In case CONFIG_ARM_MODULE_PLTS is not enabled what would happen? Is it
> impossible to hit this corner case or we could fail one way or another? IOW,
> should this check be always compiled in?
I didn't want to modify the original behavior and the limits are again checked
in either ARM or THUMB implementations of __arm_gen_branch() (there you will
again find a nice set of "magic numbers".
>> +
>> return arm_gen_branch_link(pc, addr);
>> }
>>
>> @@ -124,10 +137,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>> {
>> unsigned long new, old;
>> unsigned long ip = rec->ip;
>> + unsigned long aaddr = adjust_address(rec, addr);
>>
>> old = ftrace_nop_replace(rec);
>>
>> - new = ftrace_call_replace(ip, adjust_address(rec, addr));
>> + new = ftrace_call_replace(ip, aaddr);
>> +
>> +#ifdef CONFIG_ARM_MODULE_PLTS
>> + if (!new) {
>> + struct module *mod = rec->arch.mod;
>> +
>> + if (mod) {
>
> What would happen if !new and !mod?
I believe, that's exactly what happens in the dump you experience with your kernel.
This is not covered by this patch, this patch covers the issue with modules in vmalloc area.
>> + aaddr = get_module_plt(mod, ip, aaddr);
>> + new = ftrace_call_replace(ip, aaddr);
>
> I assume we're guaranteed to have a sensible value returned in 'new' here?
Otherwise you'd see the dump you see :)
It relies on the already existing error handling.
>> + }
>> + }
>> +#endif
--
Best regards,
Alexander Sverdlin.
Powered by blists - more mailing lists