linux-kernel - Re: [PATCH] kho: make sure folio being restored is actually from KHO

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: <mafs0zfauh6to.fsf@kernel.org>
Date: Tue, 16 Sep 2025 15:20:51 +0200
From: Pratyush Yadav <pratyush@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Pratyush Yadav <me@...avpratyush.com>,  Matthew Wilcox
 <willy@...radead.org>,  Pratyush Yadav <pratyush@...nel.org>,  Alexander
 Graf <graf@...zon.com>,  Mike Rapoport <rppt@...nel.org>,  Changyuan Lyu
 <changyuanl@...gle.com>,  Andrew Morton <akpm@...ux-foundation.org>,
  Baoquan He <bhe@...hat.com>,  Pasha Tatashin <pasha.tatashin@...een.com>,
  Chris Li <chrisl@...nel.org>,  Jason Miu <jasonmiu@...gle.com>,
  linux-kernel@...r.kernel.org,  kexec@...ts.infradead.org,
  linux-mm@...ck.org
Subject: Re: [PATCH] kho: make sure folio being restored is actually from KHO

On Wed, Sep 10 2025, Jason Gunthorpe wrote:

> On Wed, Sep 10, 2025 at 05:52:04PM +0200, Pratyush Yadav wrote:
>> On Wed, Sep 10 2025, Matthew Wilcox wrote:
>> 
>> > On Wed, Sep 10, 2025 at 05:34:40PM +0200, Pratyush Yadav wrote:
>> >> +#define KHO_PAGE_MAGIC 0x4b484f50U /* ASCII for 'KHOP' */
>> >> +
>> >> +/*
>> >> + * KHO uses page->private, which is an unsigned long, to store page metadata.
>> >> + * Use it to store both the magic and the order.
>> >> + */
>> >> +union kho_page_info {
>> >> +	unsigned long page_private;
>> >> +	struct {
>> >> +		unsigned int order;
>> >> +		unsigned int magic;
>> >> +	};
>> >
>> > KHO is only supported on 64-bit?
>> 
>> Yes. Currently only x86_64 and ARM64. It is mainly for hypervisor live
>> update so there isn't much reason to support it on 32-bit platforms.
>
> Presumably this will eventually change to use some special coding on the memdesc
> pointer?

Maybe. I didn't think that through yet.

>
>> >> @@ -210,16 +226,16 @@ static void kho_restore_page(struct page *page, unsigned int order)
>> >>  struct folio *kho_restore_folio(phys_addr_t phys)
>> >>  {
>> >>  	struct page *page = pfn_to_online_page(PHYS_PFN(phys));
>> >> -	unsigned long order;
>> >> +	union kho_page_info info;
>> >>  
>> >>  	if (!page)
>> >>  		return NULL;
>> >>  
>> >> -	order = page->private;
>> >> -	if (order > MAX_PAGE_ORDER)
>> >> +	info.page_private = page->private;
>> >> +	if (info.magic != KHO_PAGE_MAGIC || info.order > MAX_PAGE_ORDER)
>
> All the impossible checks shoudl be WARN_ON()

The mental model I have is that the place that introduced the
"impossible" situation should get the WARN(). So for an incorrect phys
address (leading to magic mismatch) or incorrect order (say preserved
big range using kho_preserve_phys() and restoring it using
kho_restore_folio()), the caller is responsible so it should do the
WARN(). Does that make sense?

>
>> >>  		return NULL;
>> >>  
>> >> -	kho_restore_page(page, order);
>> >> +	kho_restore_page(page, info.order);
>> >>  	return page_folio(page);
>> >
>> > This all looks very confused.  Before your patch as well as after it.
>> > I don't see anything in the current KHO code that requires the
>> > phys_addr_t to be order-aligned.
>> 
>> Right, good point. I can send that as a follow up patch. But I think
>> this patch stands on its own without that fix too.

Actually, on another look, this patch implicitly makes sure that
unaligned phys always fails. This is because deserialize_bitmap() only
sets the magic on the head page which is always aligned by the order.
For any unaligned phys, the magic will not match.

Still, would be a good idea to do a KHO_DEBUG check for alignment if
this implicit thing ever changes.

>
> Maybe it is worth adding some KHO_DEBUG kconfig to protect some of
> these extra checks?
>
> phys should be pfn_valid, phys should be aligned, the page should be
> in the right state, order should be valid, etc. All worth checking.

Makes sense. Do you suggest the magic and order checks to also be under
KHO_DEBUG, or should they always be done? I am of the opinion that it
makes sense to do them always, but I can also understand the argument
for disabling them in production for better performance.

-- 
Regards,
Pratyush Yadav