linux-kernel - Re: [tip: x86/mm] x86/mm: Clear _PAGE_DIRTY when we clear _PAGE

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: <CAHk-=whfkWMkQOVMCxqcJ6+GWdSZTLcyDUmSRCVHV4BtbwDrHA@mail.gmail.com>
Date: Tue, 25 Feb 2025 13:07:08 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: linux-kernel@...r.kernel.org
Cc: linux-tip-commits@...r.kernel.org, 
	kernel test robot <oliver.sang@...el.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>, 
	Ingo Molnar <mingo@...nel.org>, x86@...nel.org
Subject: Re: [tip: x86/mm] x86/mm: Clear _PAGE_DIRTY when we clear _PAGE_RW

On Tue, 25 Feb 2025 at 12:10, tip-bot2 for Matthew Wilcox (Oracle)
<tip-bot2@...utronix.de> wrote:
>
> We should, therefore, clear the _PAGE_DIRTY bit whenever we clear
> _PAGE_RW.  I opted to do it in the callers in case we want to use
> __change_page_attr() to create shadow stacks inside the kernel at some
> point in the future.  Arguably, we might also want to clear _PAGE_ACCESSED
> here.

This explanation makes ZERO sense, and screams "this is a major bug" to me.

If a page is dirty, it doesn't magically turn clean just because it
becomes read-only. The dirty data remains and may need to be written
back to memory.

Imagine writing to a shared memory area, and then marking it all
read-only after you're done. It's still dirty, even if it's read-only.

Now, I don't actually expect this patch to be wrong, I'm literally
just complaining about the explanation. Because the explanation is
very lacking. That's particularly true for the __set_pages_np() case
which also clears _PAGE_PRESENT, because then the whole shadow stacks
explanation flies right out the window: the shadow stack rules simply
do NOT APPLY to non-present pte's in the first place.

So honestly, I think this wants an explanation for why it's actually a
safe change, and how the dirty bit has been saved before the
operation.

             Linus