linux-kernel - RE: [PATCH] efi: Free existing memory map before installing new memory map

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, 27 Jun 2018 04:51:27 +0000
From:   "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@...el.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
CC:     linux-efi <linux-efi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lee Chun-Yi <jlee@...e.com>, Borislav Petkov <bp@...en8.de>,
        Dave Young <dyoung@...hat.com>,
        Laszlo Ersek <lersek@...hat.com>,
        Bhupesh Sharma <bhsharma@...hat.com>,
        "Neri, Ricardo" <ricardo.neri@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        Matt Fleming <matt@...eblueprint.co.uk>
Subject: RE: [PATCH] efi: Free existing memory map before installing new
 memory map

> > +       /* Free the memory allocated to the existing memory map */
> > +       efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
> > + efi.memmap.late);
> > +
> >         data.phys_map = addr;
> >         data.size = efi.memmap.desc_size * nr_map;
> >         data.desc_version = efi.memmap.desc_version;
> > --
> > 2.7.4
> >
> 
> If only it were so simple :-)
> 
> At this point, efi.memmap.phys_map could point to memory that was allocated
> early, allocated late or simply passed to the OS at boot time by the stub (in
> which case it was memblock_reserve()d but not memblock_alloc()d, and it
> should not be freed)
> 

Yes, completely agree that there could be three types of allocations for memmap. 
I thought, 
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);

should work because the previous type of allocation should have been recorded in efi.memmap.late.
But, now I see this will fail for memblock_reserved() memory because it will be mistaken to 
memblock_alloced() (I assumed both are almost similar :().

> So only allocations made with efi_memmap_alloc() should be freed here.

Makes sense and I think that also means efi_memmap_free() should be called from function 
that called efi_memmap_alloc() and not efi_memmap_install().

> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of
> replacing the boolean 'late' with an enum that describes where the memory
> came from that phys_map points to.

I did try changing boolean late to enum and it seems to be working fine. I will do more 
testing/clean up and will submit a patch for review.

Also, could you please clarify if there is any specific reason why memory allocated 
using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I 
think we could make it _available_ using free_bootmem() (or something similar, please 
correct me if this is not the right API). If we allocate and install a new memory map (as 
in case with efi_fake_memmap()), I think we should free the memory used by memory map 
originally passed by EFI stub, because, at any point of time there should only be one active 
memory map. If we don't free the original memory map passed by EFI stub, we will be having
two and hence will be leaking memory.

Regards,
Sai