linux-kernel - Re: [PATCH] x86/mm: fix lookup_address() to handle physical memory holes in direct mapping

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]

Date: Sat, 29 Jun 2024 10:16:26 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Jürgen Groß <jgross@...e.com>,
 Ashish Kalra <Ashish.Kalra@....com>, dave.hansen@...ux.intel.com,
 luto@...nel.org, peterz@...radead.org, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de
Cc: x86@...nel.org, hpa@...or.com, kirill.shutemov@...ux.intel.com,
 rick.p.edgecombe@...el.com, mhklinux@...look.com, peterx@...hat.com,
 linux-kernel@...r.kernel.org, linux-coco@...ts.linux.dev, jroedel@...e.de
Subject: Re: [PATCH] x86/mm: fix lookup_address() to handle physical memory
 holes in direct mapping

On 6/29/24 05:20, Jürgen Groß wrote:
> On 28.06.24 22:52, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@....com>
>>
>> lookup_address_in_pgd_attr() at pte level it is simply returning
>> pte_offset_kernel() and there does not seem to be a check for
>> returning NULL if pte_none().
>>
>> Fix lookup_address_in_pgd_attr() to add check for pte_none()
>> after pte_offset_kernel() and return NULL if it is true.
> 
> Please have a look at the comment above lookup_address(). You should not
> break the documented behavior without verifying that no caller is relying
> on the current behavior. If this is fine, please update the comment.

This brings up a point from my other reply. The comment says that it
returns "the effective NX and RW bits of all page table levels", but in
fact NX and RW are not updated for the PTE. Since the comment says all
page table levels, shouldn't they be updated with the PTE values, too?

Thanks,
Tom

> 
> 
> Juergen
> 
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>> ---
>>   arch/x86/mm/pat/set_memory.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
>> index 443a97e515c0..be8b5bf3bc3f 100644
>> --- a/arch/x86/mm/pat/set_memory.c
>> +++ b/arch/x86/mm/pat/set_memory.c
>> @@ -672,6 +672,7 @@ pte_t *lookup_address_in_pgd_attr(pgd_t *pgd,
>> unsigned long address,
>>       p4d_t *p4d;
>>       pud_t *pud;
>>       pmd_t *pmd;
>> +    pte_t *pte;
>>         *level = PG_LEVEL_256T;
>>       *nx = false;
>> @@ -717,7 +718,11 @@ pte_t *lookup_address_in_pgd_attr(pgd_t *pgd,
>> unsigned long address,
>>       *nx |= pmd_flags(*pmd) & _PAGE_NX;
>>       *rw &= pmd_flags(*pmd) & _PAGE_RW;
>>   -    return pte_offset_kernel(pmd, address);
>> +    pte = pte_offset_kernel(pmd, address);
>> +    if (pte_none(*pte))
>> +        return NULL;
>> +
>> +    return pte;
>>   }
>>     /*
>