linux-kernel - Re: [PATCH v2] mm/swapfile: unuse_pte can map random data if swap read fails

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:   Tue, 19 Apr 2022 13:46:56 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Miaohe Lin <linmiaohe@...wei.com>, akpm@...ux-foundation.org
Cc:     willy@...radead.org, vbabka@...e.cz, dhowells@...hat.com,
        neilb@...e.de, apopple@...dia.com, surenb@...gle.com,
        minchan@...nel.org, peterx@...hat.com, sfr@...b.auug.org.au,
        rcampbell@...dia.com, naoya.horiguchi@....com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mm/swapfile: unuse_pte can map random data if swap
 read fails

On 19.04.22 13:21, Miaohe Lin wrote:
> On 2022/4/19 15:37, David Hildenbrand wrote:
>> On 16.04.22 05:05, Miaohe Lin wrote:
>>> There is a bug in unuse_pte(): when swap page happens to be unreadable,
>>> page filled with random data is mapped into user address space. In case
>>> of error, a special swap entry indicating swap read fails is set to the
>>> page table. So the swapcache page can be freed and the user won't end up
>>> with a permanently mounted swap because a sector is bad. And if the page
>>> is accessed later, the user process will be killed so that corrupted data
>>> is never consumed. On the other hand, if the page is never accessed, the
>>> user won't even notice it.
>>>
>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>>> ---
>>> v2:
>>>   use special swap entry to avoid permanently mounted swap
>>>   free the bad page in swapcache
>>> ---
>>>  include/linux/swap.h    |  7 ++++++-
>>>  include/linux/swapops.h | 10 ++++++++++
>>>  mm/memory.c             |  5 ++++-
>>>  mm/swapfile.c           | 11 +++++++++++
>>>  4 files changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>>> index d112434f85df..03c576111737 100644
>>> --- a/include/linux/swap.h
>>> +++ b/include/linux/swap.h
>>> @@ -55,6 +55,10 @@ static inline int current_is_kswapd(void)
>>>   * actions on faults.
>>>   */
>>>  
>>> +#define SWAP_READ_ERROR_NUM 1
>>> +#define SWAP_READ_ERROR     (MAX_SWAPFILES + SWP_HWPOISON_NUM + \
>>> +			     SWP_MIGRATION_NUM + SWP_DEVICE_NUM + \
>>> +			     SWP_PTE_MARKER_NUM)
>>
>> Does anything speak against reusing the hwpoison marker? At least from a
>> program POV it's similar "the previously well defined content at this
>> user space address is no longer readable/writable".
> 
> Looks like a good idea. :)
> 
>>
>> I recall that we can just set the pfn to 0 for the hwpoison marker.
>>
>> There is e.g., check_hwpoisoned_entry() and it just stops if it finds
>> "pfn=0".
> 
> Do you mean that we should set the pfn to 0 for the hwpoison marker so that we can
> distinguish swapin error case from real hwpoison case?

I am not sure if we really have to distinguish. However, "0" seems to
make sense to indicate "this is not an actual problematic PFN, the
information is simply no longer around due to a hardware issue.

-- 
Thanks,

David / dhildenb