Message-ID: <14089153-a5a9-1335-94f8-158ae4bbaee0@gmail.com>
Date:   Sun, 29 Apr 2018 20:41:32 +0400
From:   Igor Stoppa <igor.stoppa@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     mhocko@...nel.org, akpm@...ux-foundation.org,
        keescook@...omium.org, linux-mm@...ck.org,
        kernel-hardening@...ts.openwall.com,
        linux-security-module@...r.kernel.org, labbott@...hat.com,
        linux-kernel@...r.kernel.org, igor.stoppa@...wei.com
Subject: Re: [PATCH 0/3] linux-next: mm: hardening: Track genalloc allocations

oops, sorry, I forgot the references :-(

On 29/04/18 20:39, Igor Stoppa wrote:
> 
> 
> On 29/04/18 07:09, Matthew Wilcox wrote:
>> On Sun, Apr 29, 2018 at 06:45:39AM +0400, Igor Stoppa wrote:
>>> This patchset was created as part of an older version of pmalloc, 
>>> however
>>> it has value per-se, as it hardens the memory management for the generic
>>> allocator genalloc.
>>>
>>> Genalloc does not currently track the size of the allocations it 
>>> hands out.
>>>
>>> Either by mistake, or due to an attack, it is possible that more memory
>>> than what was initially allocated is freed, leaving behind dangling
>>> pointers, ready for an use-after-free attack.
>>
>> This is a good point.  It is worth hardening genalloc.
>> But I still don't like the cost of the bitmap.  I've been
>> reading about allocators and I found Bonwick's paper from 2001:
>> https://www.usenix.org/legacy/event/usenix01/full_papers/bonwick/bonwick.pdf 
>>
>> Section 4 describes the vmem allocator which would seem to have superior
>> performance and lower memory overhead than the current genalloc 
>> allocator,
>> never mind the hardened allocator.
>>
>> Maybe there's been an advnace in resource allocator technology since
>> then that someone more familiar with CS research can point out.
> 
> A quick search on google shows that there have been tons of improvements.
> 
> I found various implementation of vmem, not all with GPLv2 compatible 
> license.
> 
> The most interesting one seems to be a libvmem from Intel[1], made to 
> use jemalloc[2], for persistent memory.
> 
> jemalloc is, apaprently, the coolest kid on the block, when it comes to 
> modern memory management.
> 
> But this is clearly a very large lump of work.
> 
> First of all, it should be assessed if jemalloc is really what the 
> kernel could benefit from (my guess is yes, but it's just a guess), then 
> if the license is compatible or if it can be relicensed for use in the 
> kernel.
> 
> And, last, but not least, how to integrate the ongoing work in a way 
> that doesn't require lots of effort to upgrade to new releases.
> 
> Even if it looks very interesting, I simply do not have time to do this, 
> not for the next 5-6 months, for sure.
> 
> What I *can* offer to do, is the cleanup of the users of genalloc, by 
> working with the various maintainers to remove the "size" parameter in 
> the calls to gen_pool_free(), iff the patch I submitted can be merged.
> 
> This is work that has to be done anyway and does not preclude, later on, 
> to phase out genalloc in favor of jemalloc or whatever is deemed to be 
> the most effective solution.
> 
> There are 2 goals here:
> 1) plug potential security holes in the use of genalloc
> 2) see if some new allocator can improve the performance (and it might 
> well be that the improvement can be extended also to other allocators 
> used in kernel)
> 
> We seem to agree that 1) is a real need.
> Regarding 2), I think it should have a separate life.
> 
> going back to 1), your objections so far, as far as I can tell are:
> 
> a) it will use more memory for the bitmap
> b) it will be slower, because the bitmap is doubled
> c) the "ends" or "starts" bitmaps should be separate
> 
> I think I have already answered them, but I'll recap my replies:
> 
> a) yes, it will double it, but if it was ok to "waste" some memory when 
> I was asked to rewrite the pmalloc allocator to not use genalloc, in 
> favor of speed, I think the same criteria applies here: on average it 
> will probably take at most one more page per pool. It doesn't seem a 
> huge loss.
> 
> b) the bitmap size is doubled, that much is true, however interleaving 
> the "busy" and "start" bitmaps will ensure locality of the meta data and 
> between the cache prefetch algorithm and the hints give to the compiler, 
> it shouldn't make a huge difference, compared to the pre-patch case.
> Oh, and the size of a bitmap seems to be overall negligible, from what 
> users I saw.
> 
> c) "busy" and "start" are interleaved, to avoid having to do explicit 
> locking, instead of relying on the intrinsic atomicity of accessing 
> bitfields coming from the same word, as it is now.
> 
> And I'm anyway proposing to merge this into linux-next, so that there 
> are more eyeballs looking for problems. I'm not proposing to merge it 
> straight in the next kernel release.
> Removing the size parameter from the various gen_pool_free() will impact 
> not only the direct callers, but also their callers and so on, which 
> means that it will take some time to purge all the layers of calls from 
> "size".
> 
> During this time, it's likely that issues will surface, if there is any 
> lurking.
> 
> And the removal of the parameter will require getting ACK from each 
> user, so it should be enough to ensure that everyone is happy about the 
> overall performance.
> 
> But I would start addressing the security issue, since I think the cost 
> of doubling the bitmap will not be noticeable.
> 
> I'd like to hear if you disagree with my reasoning.
> 
> And did I overlook some other objection?
> 

[1] https://github.com/pmem/pmdk/tree/master/src/libvmem
[2] http://jemalloc.net/

Powered by blists - more mailing lists