linux-kernel - Re: [PATCH 1/1] arm64/mm: move {idmap_pg_dir,tramp_pg_dir,swapper_pg

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]

Message-ID: <CAKv+Gu-t5ErL89J=sxeVRYzDNrB1Ejdj+XOggAMWk-gY7ewsOg@mail.gmail.com>
Date:   Thu, 21 Jun 2018 11:29:52 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     James Morse <james.morse@....com>
Cc:     Jun Yao <yaojun8558363@...il.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH 1/1] arm64/mm: move {idmap_pg_dir,tramp_pg_dir,swapper_pg_dir}
 to .rodata section

On 21 June 2018 at 10:59, James Morse <james.morse@....com> wrote:
> Hi guys,
>
> On 21/06/18 07:39, Ard Biesheuvel wrote:
>> On 21 June 2018 at 04:51, Jun Yao <yaojun8558363@...il.com> wrote:
>>> On Wed, Jun 20, 2018 at 12:09:49PM +0200, Ard Biesheuvel wrote:
>>>> On 20 June 2018 at 10:57, Jun Yao <yaojun8558363@...il.com> wrote:
>>>>> Move {idmap_pg_dir,tramp_pg_dir,swapper_pg_dir} to .rodata
>>>>> section. And update the swapper_pg_dir by fixmap.
>>>>>
>>>>
>>>> I think we may be able to get away with not mapping idmap_pg_dir and
>>>> tramp_pg_dir at all.
>>>
>>> I think we need to move tramp_pg_dir to .rodata. The attacker can write
>>> a block-mapping(AP=01) to tramp_pg_dir and then he can access kernel
>>> memory.
>
>> Why does it need to be mapped at all? When do we ever access it from the code?
>
> (We would want to make its fixmap entry read-only too)
>

It already is.

>
>>>> As for swapper_pg_dir, it would indeed be nice if we could keep those
>>>> mappings read-only most of the time, but I'm not sure how useful this
>>>> is if we apply it to the root level only.
>>>
>>> The purpose of it is to make 'KSMA' harder, where an single arbitrary
>>> write is used to add a block mapping to the page-tables, giving the
>>> attacker full access to kernel memory. That's why we just apply it to
>>> the root level only. If the attacker can arbitrary write multiple times,
>>> I think it's hard to defend.
>>>
>>
>> So the assumption is that the root level is more easy to find?
>> Otherwise, I'm not sure I understand why being able to write a level 0
>> entry is so harmful, given that we don't have block mappings at that
>> level.
>
> I think this thing assumes 3-level page tables with 39bit VA.
>

The attack, you mean? Because this code is unlikely to build with that
configuration, given that __pgd_populate() BUILD_BUG()s in that case.

>
>>>>> @@ -417,12 +421,22 @@ static void __init __map_memblock(pgd_t *pgdp, phys_addr_t start,
>>>>>
>>>>>  void __init mark_linear_text_alias_ro(void)
>>>>>  {
>
>>>>> +       size = (unsigned long)__init_begin - (unsigned long)swapper_pg_end;
>>>>> +       update_mapping_prot(__pa_symbol(swapper_pg_end),
>>>>> +                           (unsigned long)lm_alias(swapper_pg_end),
>>>>> +                           size, PAGE_KERNEL_RO);
>>>>
>>>> I don't think this is necessary. Even if some pages are freed, it
>>>> doesn't harm to keep a read-only alias of them here since the new
>>>> owner won't access them via this mapping anyway. So we can keep
>>>> .rodata as a single region.
>>>
>>> To be honest, I didn't think of this issue at first. I later found a
>>> problem when testing the code on qemu:
>>
>> OK, you're right. I missed the fact that this operates on the linear
>> alias, not the kernel mapping itself.
>>
>> What I don't like is that we lose the ability to use block mappings
>> for the entire .rodata section this way. Isn't it possible to move
>> these pgdirs to the end of the .rodata segment, perhaps by using a
>> separate input section name and placing that explicitly? We could even
>> simply forget about freeing those pages, given that [on 4k pages] the
>> benefit of freeing 12 KB of space is likely to get lost in the
>> rounding noise anyway [segments are rounded up to 64 KB in size]
>
> I assumed that to move swapper_pg_dir into the .rodata section we would need to
> break it up. Today its ~3 levels, which we setup in head.S, then do a dance in
> paging_init() so that swapper_pg_dir is always the top level.
>
> We could generate all leves of the 'init_pg_dir' in the __initdata section, then
> copy only the top level into swapper_pg_dir into the rodata section during
> paging_init().
>

Is that complexity truly justified for a security sensitive piece of
code? Can't we just drop the memblock_free() and be done with it?