linux-kernel - Re: [PATCH v1] mm/madvise: Always set ptes via arch helpers

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: <2308a4d0-273e-4cf8-9c9f-3008c42b6d18@arm.com>
Date: Fri, 7 Mar 2025 14:35:12 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1] mm/madvise: Always set ptes via arch helpers

On 07/03/2025 13:59, Lorenzo Stoakes wrote:
> On Fri, Mar 07, 2025 at 01:42:13PM +0000, Ryan Roberts wrote:
>> On 07/03/2025 13:04, Lorenzo Stoakes wrote:
>>> On Fri, Mar 07, 2025 at 12:33:06PM +0000, Ryan Roberts wrote:
>>>> Instead of writing a pte directly into the table, use the set_pte_at()
>>>> helper, which gives the arch visibility of the change.
>>>>
>>>> In this instance we are guaranteed that the pte was originally none and
>>>> is being modified to a not-present pte, so there was unlikely to be a
>>>> bug in practice (at least not on arm64). But it's bad practice to write
>>>> the page table memory directly without arch involvement.
>>>>
>>>> Cc: <stable@...r.kernel.org>
>>>> Fixes: 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism")
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
>>>> ---
>>>>  mm/madvise.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>>> index 388dc289b5d1..6170f4acc14f 100644
>>>> --- a/mm/madvise.c
>>>> +++ b/mm/madvise.c
>>>> @@ -1101,7 +1101,7 @@ static int guard_install_set_pte(unsigned long addr, unsigned long next,
>>>>  	unsigned long *nr_pages = (unsigned long *)walk->private;
>>>>
>>>>  	/* Simply install a PTE marker, this causes segfault on access. */
>>>> -	*ptep = make_pte_marker(PTE_MARKER_GUARD);
>>>> +	set_pte_at(walk->mm, addr, ptep, make_pte_marker(PTE_MARKER_GUARD));
>>>
>>> I agree with you, but I think perhaps the arg name here is misleading :) If
>>> you look at mm/pagewalk.c and specifically, in walk_pte_range_inner():
>>>
>>> 		if (ops->install_pte && pte_none(ptep_get(pte))) {
>>> 			pte_t new_pte;
>>>
>>> 			err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte,
>>> 					       walk);
>>> 			if (err)
>>> 				break;
>>>
>>> 			set_pte_at(walk->mm, addr, pte, new_pte);
>>>
>>> 			...
>>> 		}
>>>
>>> So the ptep being assigned here is a stack value, new_pte, which we simply
>>> assign to, and _then_ the page walker code handles the set_pte_at() for us.
>>>
>>> So we are indeed doing the right thing here, just in a different place :P
>>
>> Ahh my bad. In that case, please ignore the patch.
>>
>> But out of interest, why are you doing it like this? I find it a bit confusing
>> as all the other ops (e.g. pte_entry()) work directly on the pgtable's pte
>> without the intermediate.
> 
> In those cases it's read-only, the data's already there, you can just go ahead
> and manipulate it (and would expect to be able to do so).

It's certainly not read-only in general. Just having a quick look to verify, the
very first callback I landed on was clear_refs_pte_range(), which implements
.pmd_entry to clear the softdirty and access flags from a leaf pmd or from all
the child ptes.

> 
> When setting things are a little different, I'd rather not open up things to a
> user being able to do *whatever*, but rather limit to the smallest scope
> possible for installing the PTE.

Understandable, but personally I think it will lead to potential misunderstandings:

 - it will get copy/pasted as an example of how to set a pte (which is wrong;
you have to use set_pte_at()/set_ptes()). There is currently only a single other
case of direct dereferencing a pte to set it (in write_protect_page()).

 - new users of .install_pte may assume (like I did) that the passed in ptep is
pointing to the pgtable and they will manipulate it with arch helpers. arm64
arch helpers all assume they are only ever passed pointers into pgtable memory.
It will end horribly if that is not the case.

> 
> And also of course, it allows us to _mandate_ that set_pte_at() is used so we do
> the right thing re: arches :)
> 
> I could have named the parameter better though, in guard_install_pte_entry()
> would be better to have called it 'new_pte' or something.

I'd suggest at least describing this in the documentation in pagewalk.h. Or
better yet, you could make the pte the return value for the function. Then it is
clear because you have no pointer. You'd lose the error code but the only user
of this currently can't fail anyway.

Anyway, just my 2 pence.

Thanks,
Ryan

> 
>>
>> Thanks,
>> Ryan
>>
>>>
>>>>  	(*nr_pages)++;
>>>>
>>>>  	return 0;
>>>> --
>>>> 2.43.0
>>>>
>>
> 
> Thanks for looking at this by the way, obviously I appreciate your point in
> chasing up cases like this as endeavoured to do the right thing here, albeit
> abstracted away :)