Re: MPK: pkey_free and key reuse

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/08/2017 09:41 PM, Dave Hansen wrote:
On 11/05/2017 02:35 AM, Florian Weimer wrote:
I don't think pkey_free, as it is implemented today, is very safe due to
key reuse by a subsequent pkey_alloc.  I see two problems:

(A) pkey_free allows reuse for they key while there are still mappings
that use it.

I don't agree with this assessment.  Is malloc() unsafe?  If someone
free()s memory that is still in use, a subsequent malloc() would hand
the address out again for reuse.

I think the disagreement is not about what is considered acceptable behavior as such, but what constitutes “use”.

And even if with concurrent use, the behavior can be well-defined. We make sure that if munmap is called, we do not return before all threads have observed in principle that the page is gone (at considerable cost, of course, and in most cases, that is total overkill).

I'm pretty sure there is another key reuse scenario which does not even involve pkey_free, but I need to write a test first.

(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Where does this expectation come from?

For me, it was the access_rights argument to pkey_alloc. What else would it do? For the current thread, I can already set the rights with a PKRU write, so the existence of the syscall argument is puzzling.

Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.

But malloc/free isn't expected to be a partial antidote to random pointer scribbling.

We define free() as only being called on resources to which there are no
active references.  If you free() things in use, bad things happen.
pkey_free() is only to be called when there is nothing actively using
the key.  If you pkey_free() an in-use key, bad things happen.

My impression was that MPK was intended as a fallback in case you did that, and unrelated code suddenly writes through a dangling pointer and accidentally hits the DAX-mapped persistent memory of the database. To prevent that, the those pages are mapped write-disabled on all threads almost all the time, and only if the database needs to write something, it temporarily tweaks PKRU so that it gains access. All that assumes that you can actually restrict all threads in the process, but with the current implementation, that's not true even if threads never touch keys they don't know.

I think we should either implement revoke on pkey_alloc, with a broadcast to all threads (the pkey_set race can be closed by having a vDSO for that an the revocation code can check %rip to see if the old PKRU value needs to be fixed up). Or we add the two pkey_alloc flags I mentioned earlier.

Thanks,
Florian



[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux