linux-kernel - Re: [PATCH v4 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: <b75653b8-5264-ca03-bf5c-671e07e7fdd8@collabora.com>
Date:   Sat, 19 Nov 2022 01:16:26 +0500
From:   Muhammad Usama Anjum <usama.anjum@...labora.com>
To:     Peter Xu <peterx@...hat.com>, David Hildenbrand <david@...hat.com>
Cc:     Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()

Hi Peter and David,

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? I'm missing some obvious thing.
Maybe the meaning of tracking is to see if VM_SOFTDIRTY needs to be set. If
VM_SOFTDIRTY is already set, tracking isn't needed. Can you give an example
here?

-- 
BR,
Muhammad Usama Anjum