| 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
| ||
|
Message-ID: <e3c26b56-5458-eb25-c753-fc4c058ba1b1@collabora.com>
Date: Mon, 19 Dec 2022 17:19:12 +0500
From: Muhammad Usama Anjum <usama.anjum@...labora.com>
To: Peter Xu <peterx@...hat.com>, David Hildenbrand <david@...hat.com>,
Cyrill Gorcunov <gorcunov@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Paul Gofman <pgofman@...eweavers.com>
Cc: Muhammad Usama Anjum <usama.anjum@...labora.com>,
Nadav Amit <nadav.amit@...il.com>,
Andrea Arcangeli <aarcange@...hat.com>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
kernel@...labora.com
Subject: Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in
can_change_pte_writable()
On 11/22/22 2:17 AM, Peter Xu wrote:
> On Mon, Nov 21, 2022 at 07:57:05PM +0500, Muhammad Usama Anjum wrote:
>> Hi Peter,
>>
>> Thank you so much for replying.
>>
>> On 11/19/22 4:14 AM, Peter Xu wrote:
>>> On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote:
>>>> Hi Peter and David,
>>>
>>> Hi, Muhammad,
>>>
>>>>
>>>> On 7/25/22 7:20 PM, Peter Xu wrote:
>>>>> The check wanted to make sure when soft-dirty tracking is enabled we won't
>>>>> grant write bit by accident, as a page fault is needed for dirty tracking.
>>>>> The intention is correct but we didn't check it right because VM_SOFTDIRTY
>>>>> set actually means soft-dirty tracking disabled. Fix it.
>>>> [...]
>>>>> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
>>>>> +{
>>>>> + /*
>>>>> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty
>>>>> + * enablements, because when without soft-dirty being compiled in,
>>>>> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY)
>>>>> + * will be constantly true.
>>>>> + */
>>>>> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>>>>> + return false;
>>>>> +
>>>>> + /*
>>>>> + * Soft-dirty is kind of special: its tracking is enabled when the
>>>>> + * vma flags not set.
>>>>> + */
>>>>> + return !(vma->vm_flags & VM_SOFTDIRTY);
>>>>> +}
>>>> I'm sorry. I'm unable to understand the inversion here.
>>>>> its tracking is enabled when the vma flags not set.
>>>> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is
>>>> soft-dirty. When we write to clear_refs to clear soft-dirty bit,
>>>> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking
>>>> is enabled when the vma flags not set?
>>>
>>> Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and
>>> only until then the real tracking starts (by removing write bits on ptes).
>> But even if the VM_SOFTDIRTY is set on the VMA, the individual pages are
>> still marked soft-dirty. Both are independent.
>>
>> It means tracking is enabled all the time in individual pages.
Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE
bit status setting. The internal behavior has changed. The test case was
shared by David
(https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/).
The explanation is as following:
_Before_ addition of this patch(76aefad628aae),
m = mmap(2 pages)
clear_softdirty()
mremap(m + pag_size)
mprotect(READ)
mprotect(READ | WRITE);
memset(m)
After memset(),
PAGE-1 PAGE-2
VM_SOFTDIRTY set set
PTE softdirty flag set set
/proc//pagemap view set set
_After_ addition of this patch(76aefad628aae)
m = mmap(2 pages)
clear_softdirty()
mremap(m + page_size)
mprotect(READ)
mprotect(READ | WRITE);
memset(m)
After memset(),
PAGE-1 PAGE-2
VM_SOFTDIRTY set set
PTE softdirty flag *not set* set
/proc//pagemap view set set
The user's point of view hasn't changed. But internally after this patch,
the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The
soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect
is used. Why? Because soft-dirty tracking in the PTEs is always enabled
regardless of VM_SOFTDIRTY is set or not. Example:
m = mem(2 pages)
At this point:
PAGE-1 PAGE-2
VM_SOFTDIRTY set set
PTE softdirty flag not set not set
/proc//pagemap view set set
memset(m)
At this point:
PAGE-1 PAGE-2
VM_SOFTDIRTY set set
PTE softdirty flag set set
/proc//pagemap view set set
This example proves that soft-dirty flag on the PTE is set regardless of
the VM_SOFTDIRTY.
The simplest hack to get rid this changed behavior and revert to the
previous behaviour is as following:
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -860,6 +860,8 @@ static inline bool vma_soft_dirty_enabled(struct
vm_area_struct *vma)
if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
return false;
+ return true;
+
/*
* Soft-dirty is kind of special: its tracking is enabled when the
* vma flags not set.
I was trying to verify this hack. But I couldn't previously until @Paul has
mentioned this again. I've verified with limited tests that this hack
in-deed works. We are unaware that does this hack create problems in other
areas or not. We can think of better way to solve this. Once we get the
comments from the community.
This internal behavior change is affecting the new feature addition to the
soft-dirty flag which is already delicate and has issues.
(https://lore.kernel.org/all/20221109102303.851281-1-usama.anjum@collabora.com/)
>
> IMHO it depends on how we define "tracking enabled" - before clear_refs
> even if no pages written they'll also be reported as dirty, then the
> information is useless.
>
>> Only the soft-dirty bit status in individual page isn't significant if
>> VM_SOFTDIRTY already is set. Right?
>
> Yes. But I'd say it makes more sense to say "tracking enabled" if we
> really started tracking (by removing the write bits in ptes, otherwise we
> did nothing). When vma created we didn't track anything.
>
> I don't know the rational of why soft-dirty was defined like that. I think
> it's somehow related to the fact that we allow false positive dirty pages
> not false negative. IOW, it's a bug to leak a page being dirtied, but not
> vice versa if we report clean page dirty.
>
--
BR,
Muhammad Usama Anjum
Powered by blists - more mailing lists