linux-kernel - Re: [PATCH] jump_label: align jump

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Mar 2017 15:02:52 -0500
From:   Jason Baron <jbaron@...mai.com>
To:     David Daney <ddaney@...iumnetworks.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sachin Sant <sachinp@...ux.vnet.ibm.com>
Cc:     linux-mips@...ux-mips.org, Chris Metcalf <cmetcalf@...lanox.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Russell King <linux@...linux.org.uk>,
        Rabin Vincent <rabin@....in>,
        Paul Mackerras <paulus@...ba.org>,
        Anton Blanchard <anton@...ba.org>,
        linuxppc-dev@...ts.ozlabs.org, Ingo Molnar <mingo@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, Zhigang Lu <zlu@...hip.com>
Subject: Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes



On 03/01/2017 11:40 AM, David Daney wrote:
> On 02/28/2017 10:34 PM, Michael Ellerman wrote:
>> Jason Baron <jbaron@...mai.com> writes:
>> ...
>>> I also checked all the other .ko files and they were properly aligned.
>>> So I think this should hopefully work, and I like that its not a
>>> per-arch fix.
>>>
>>> Sachin, sorry to bother you again, but I'm hoping you can try David's
>>> latest patch to scripts/module-common.lds, just to test in your setup.
>>
>> It does fix the problem.
>>
>> I was reproducing with crc_t10dif:
>>
>> [  695.890552] ------------[ cut here ]------------
>> [  695.890709] WARNING: CPU: 15 PID: 3019 at
>> ../kernel/jump_label.c:287 static_key_set_entries+0x74/0xa0
>> [  695.890710] Modules linked in: crc_t10dif(+) crct10dif_generic
>> crct10dif_common ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat
>> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
>> iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack
>> bridge stp llc dm_thin_pool dm_persistent_data dm_bio_prison dm_bufio
>> libcrc32c kvm virtio_balloon binfmt_misc autofs4 virtio_net virtio_pci
>> virtio_ring virtio
>>
>> Which had:
>>
>>   [21] __jump_table      PROGBITS        0000000000000000 0004e8
>> 000018 00  WA  0   0  1
>>
>>
>> And now has:
>>
>>   [18] __jump_table      PROGBITS        0000000000000000 0004d0
>> 000018 00  WA  0   0  8
>>
>> And all other modules have an alignment of 8 on __jump_table, as
>> expected.
>>
>> I'm inclined to merge a version of the balign patch for powerpc anyway,
>> just to be on the safe side. I guess the old code was coping fine with
>> the unaligned keys, but it still makes me nervous.
> 
> 
> The original "balign patch" has a couple of problems:
> 
> 1) 4-byte alignment is not sufficient for 64-bit kernels
> 
> 2) It is redundant if the linker script patch is accepted.
> 
> 

The linker script patch seems reasonable to me.

Maybe its worth adding a comment that the alignment is necessary because
the core jump_label makes use of the 2 lsb bits of its __jump_table
pointer due to commit:

3821fd3 jump_label: Reduce the size of struct static_key

Also, in the comment it says that it fixes an oops. We hit a WARN_ON()
not an oops, although bad things are likely to happen when the branch is
updated.

Thanks,

-Jason