linux-kernel - Re: [PATCH v2 3/3] riscv: Fix crash when flushing executable ioremap regions

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: <441527ef-1fd4-ed98-8381-8902c4e05fc5@ghiti.fr>
Date:   Sun, 16 Feb 2020 14:56:10 -0500
From:   Alex Ghiti <alex@...ti.fr>
To:     Jan Kiszka <jan.kiszka@....de>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv@...ts.infradead.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] riscv: Fix crash when flushing executable ioremap
 regions

On 2/16/20 11:05 AM, Jan Kiszka wrote:
> On 16.02.20 15:41, Alex Ghiti wrote:
>> Hi Jan,
>>
>> On 2/15/20 6:49 AM, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@...mens.com>
>>>
>>> Those are not backed by page structs, and pte_page is returning an
>>> invalid pointer.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@...mens.com>
>>> =2D--
>>>   arch/riscv/mm/cacheflush.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
>>> index 8930ab7278e6..9ee2c1a387cc 100644
>>> =2D-- a/arch/riscv/mm/cacheflush.c
>>> +++ b/arch/riscv/mm/cacheflush.c
>>> @@ -84,7 +84,8 @@ void flush_icache_pte(pte_t pte)
>>>   {
>>>       struct page *page =3D pte_page(pte);
>>>
>>> -    if (!test_and_set_bit(PG_dcache_clean, &page->flags))
>>> +    if (!pfn_valid(pte_pfn(pte)) ||
>>> +        !test_and_set_bit(PG_dcache_clean, &page->flags))
>>>           flush_icache_all();
>>>   }
>>>   #endif /* CONFIG_MMU */
>>> =2D-
>>> 2.16.4
>>>
>>>
>>
>> When did you encounter such a situation ? i.e. executable code that is
>> not backed by struct page ?
>>
>> Riscv uses the generic implementation of ioremap and the way
>> _PAGE_IOREMAP is defined does not allow to map executable memory region
>> using ioremap, so I'm interested to understand how we end up in
>> flush_icache_pte for an executable region not backed by any struct page.
> 
> You can create executable mappings of memory that Linux does not
> initially consider as RAM via ioremap_prot or ioremap_page_range. We are
> using that in Jailhouse to load the hypervisor code into reserved memory
> that is ioremapped for the purpose. Works fine on x86, arm and arm64.
> 
> Jan

Ok thanks, I had missed this API.

Regarding your patch, I find it weird to do anything if the pfn is 
invalid, we could have garbage in pte pointing to an invalid region for 
example (I admit that the effect of flushing the icache would not be 
catastrophic in that situation).

I'm not saying I will come with a better solution but I'll take a deeper 
look tomorrow.

Alex