linux-kernel - Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte

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: <Yt6htg03lqj7FFU2@xz-m1.local>
Date:   Mon, 25 Jul 2022 09:59:18 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     David Hildenbrand <david@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()

On Fri, Jul 22, 2022 at 10:21:00AM -0700, Nadav Amit wrote:
> On Jul 22, 2022, at 12:08 AM, David Hildenbrand <david@...hat.com> wrote:
> 
> >> +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);
> >> +}
> > 
> > That will come in handy in other patches I'm cooking.
> 
> clear_refs_write() also comes to mind as well (for consistency; I see no
> correctness or performance issue).

I explicitly didn't touch that because current code is better..

        mas_for_each(&mas, vma, ULONG_MAX) {
                if (!(vma->vm_flags & VM_SOFTDIRTY))
                        continue;
                vma->vm_flags &= ~VM_SOFTDIRTY;
                vma_set_page_prot(vma);
        }

It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all
vma will be jumped.

If replaced with vma_soft_dirty_enabled() it'll be instead constantly false
returned.  We'll redo vma_set_page_prot() even if unnecessary.

Here if we want to add the "CONFIG_MEM_SOFT_DIRTY" into equation it can be:

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f8cd58846a28..ab6f2913b5a5 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
                }
 
                if (type == CLEAR_REFS_SOFT_DIRTY) {
+                       if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
+                               goto out_unlock;
                        mas_for_each(&mas, vma, ULONG_MAX) {
                                if (!(vma->vm_flags & VM_SOFTDIRTY))
                                        continue;

Or even at the entrance to not take the mm sem.  But it's not anything
important IMHO, so if no one asking for that I'll just leave it be.

Thanks,

-- 
Peter Xu