linux-kernel - Re: [PATCH v2 1/2] x86/efi: don't allocate memmap through memblock after mm

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: <87inpt6ce7.fsf@gmail.com>
Date:   Thu, 05 Jan 2017 11:15:28 +0100
From:   Nicolai Stange <nicstange@...il.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Nicolai Stange <nicstange@...il.com>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "x86\@kernel.org" <x86@...nel.org>,
        "linux-efi\@vger.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v2 1/2] x86/efi: don't allocate memmap through memblock after mm_init()

Ard Biesheuvel <ard.biesheuvel@...aro.org> writes:

> On 5 January 2017 at 07:42, Ingo Molnar <mingo@...nel.org> wrote:
>>
>> * Nicolai Stange <nicstange@...il.com> wrote:
>>
>>> Matt Fleming <matt@...eblueprint.co.uk> writes:
>>>
>>> > On Thu, 22 Dec, at 11:23:39AM, Nicolai Stange wrote:
>>> >> So, after memblock is gone, allocations should be done through
>>> >> the "normal"
>>> >> page allocator. Introduce a helper, efi_memmap_alloc() for this. Use
>>> >> it from efi_arch_mem_reserve() and from efi_free_boot_services() as well.
>>> >>
>>> >> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to
>>> >> avoid copying image data")
>>> >> Signed-off-by: Nicolai Stange <nicstange@...il.com>
>>>
>>> > Could you also modify efi_fake_memmap() to use your new
>>> > efi_memmap_alloc() function for consistency
>>>
>>> Sure.
>>>
>>> I'm planning to submit another set of patches addressing the (bounded)
>>> memmap leaking in anything calling efi_memmap_unmap() though. In the
>>> course of doing so, the memmap allocation sites will get touched anyway:
>>> I'll have to store some information about how the memmap's memory has
>>> been obtained.
>>
>> Will that patch be intrusive?

Yes, definitely something for 4.11+.


> Given that memblock_alloc() calls memblock_reserve() on its
> allocations, we could simply consult the memblock_reserved table to
> infer whether the allocation being freed was created with
> memblock_alloc() or with alloc_pages().

Not sure whether this would work with CONFIG_ARCH_DISCARD_MEMBLOCK=y.
This is also the reason why 2/2 is needed.

> So I don't think such a patch
> should be that intrusive. But the normal case is that the EFI memory
> map remains mapped during the lifetime of the system, and unmapping
> the EFI memory map does not necessarily imply that it should be freed.
> This is especially true on ARM systems, where the memory map is
> allocated and populated by the stub, and never modified by the kernel
> proper, and so any freeing logic in generic code should take this into
> account as well (i.e., the memory map allocation is not
> memblock_reserve()'d, nor is it allocated using alloc_pages())



>> If yes then we'll need to keep this a separate urgent patch to fix the v4.9
>> regression that Dan Williams reported. I can apply the fix to
>> efi/urgent and get
>> it to Linus straight away if you guys agree.
>>
>
> Considering the severity of the issue it solves, and the obvious
> correctness of the fix, my preference would be to spin a v3 of this
> patch taking Matt's feedback into account, and merging that as a fix
> for v4.10 with a cc stable. The 2/2 can wait a bit longer imo

Matt's Feedback included that 

  "all memblock_alloc()s should probably be PAGE_SIZE aligned like the
   fakemem code"

Unfortunately, I can't see why this would be needed. Furthermore, this
isn't currently done outside of fakemem and thus, aliging the memmap
allocations on PAGE_SIZE would be another, quite unrelated change?

So, are you Ok with only taking the other review comment, namely

  "modify efi_fake_memmap() to use your new efi_memmap_alloc() function
   for consistency"

into account for a v3?

Thanks,

Nicolai