linux-kernel - Re: [PATCH v1 04/13] kho: Verify deserialization status and fix FDT alignment access

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: <CA+CK2bALea-5wDyaU2gMYo==yOvrdj=V6FKc4g7G8KV_vgV6Mw@mail.gmail.com>
Date: Fri, 14 Nov 2025 12:21:22 -0500
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Pratyush Yadav <pratyush@...nel.org>
Cc: akpm@...ux-foundation.org, bhe@...hat.com, rppt@...nel.org, 
	jasonmiu@...gle.com, arnd@...db.de, coxu@...hat.com, dave@...ilevsky.ca, 
	ebiggers@...gle.com, graf@...zon.com, kees@...nel.org, 
	linux-kernel@...r.kernel.org, kexec@...ts.infradead.org, linux-mm@...ck.org
Subject: Re: [PATCH v1 04/13] kho: Verify deserialization status and fix FDT
 alignment access

On Fri, Nov 14, 2025 at 11:52 AM Pratyush Yadav <pratyush@...nel.org> wrote:
>
> On Fri, Nov 14 2025, Pasha Tatashin wrote:
>
> > During boot, kho_restore_folio() relies on the memory map having been
> > successfully deserialized. If deserialization fails or no map is present,
> > attempting to restore the FDT folio is unsafe.
> >
> > Update kho_mem_deserialize() to return a boolean indicating success. Use
> > this return value in kho_memory_init() to disable KHO if deserialization
> > fails. Also, the incoming FDT folio is never used, there is no reason to
> > restore it.
> >
> > Additionally, use memcpy() to retrieve the memory map pointer from the FDT.
> > FDT properties are not guaranteed to be naturally aligned, and accessing
> > a 64-bit value via a pointer that is only 32-bit aligned can cause faults.
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@...een.com>
> > ---
> >  kernel/liveupdate/kexec_handover.c | 32 ++++++++++++++++++------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> > index a4b33ca79246..83aca3b4af15 100644
> > --- a/kernel/liveupdate/kexec_handover.c
> > +++ b/kernel/liveupdate/kexec_handover.c
> > @@ -450,20 +450,28 @@ static void __init deserialize_bitmap(unsigned int order,
> >       }
> >  }
> >
> > -static void __init kho_mem_deserialize(const void *fdt)
> > +/* Return true if memory was deserizlied */
> > +static bool __init kho_mem_deserialize(const void *fdt)
> >  {
> >       struct khoser_mem_chunk *chunk;
> > -     const phys_addr_t *mem;
> > +     const void *mem_ptr;
> > +     u64 mem;
> >       int len;
> >
> > -     mem = fdt_getprop(fdt, 0, PROP_PRESERVED_MEMORY_MAP, &len);
> > -
> > -     if (!mem || len != sizeof(*mem)) {
> > +     mem_ptr = fdt_getprop(fdt, 0, PROP_PRESERVED_MEMORY_MAP, &len);
> > +     if (!mem_ptr || len != sizeof(u64)) {
> >               pr_err("failed to get preserved memory bitmaps\n");
> > -             return;
> > +             return false;
> >       }
> > +     /* FDT guarantees 32-bit alignment, have to use memcpy */
> > +     memcpy(&mem, mem_ptr, len);
>
> Perhaps get_unaligned(mem) would have been simpler?

Hm, it certainly more descriptive. I will see if I can use it.

>
> > +
> > +     chunk = mem ? phys_to_virt(mem) : NULL;
> > +
> > +     /* No preserved physical pages were passed, no deserialization */
> > +     if (!chunk)
> > +             return false;
>
> Should we disallow all kho_restore_{folio,pages}() calls too if this
> fails? Ideally those should never happen since kho_retrieve_subtree()
> will fail, so maybe as a debug aid?

Right, my thinking was that they should never happen, as they do not
have a way to know the location of folios to be restored. So,
preventing FDT access that we do does that.


>
> >
> > -     chunk = *mem ? phys_to_virt(*mem) : NULL;
> >       while (chunk) {
> >               unsigned int i;
> >
> > @@ -472,6 +480,8 @@ static void __init kho_mem_deserialize(const void *fdt)
> >                                          &chunk->bitmaps[i]);
> >               chunk = KHOSER_LOAD_PTR(chunk->hdr.next);
> >       }
> > +
> > +     return true;
> >  }
> >
> >  /*
> > @@ -1377,16 +1387,12 @@ static void __init kho_release_scratch(void)
> >
> >  void __init kho_memory_init(void)
> >  {
> > -     struct folio *folio;
> > -
> >       if (kho_in.scratch_phys) {
> >               kho_scratch = phys_to_virt(kho_in.scratch_phys);
> >               kho_release_scratch();
> >
> > -             kho_mem_deserialize(kho_get_fdt());
> > -             folio = kho_restore_folio(kho_in.fdt_phys);
> > -             if (!folio)
> > -                     pr_warn("failed to restore folio for KHO fdt\n");
> > +             if (!kho_mem_deserialize(kho_get_fdt()))
> > +                     kho_in.fdt_phys = 0;
>
> The folio restore does serve a purpose: it accounts for that folio in
> the system's total memory. See the call to adjust_managed_page_count()
> in kho_restore_page(). In practice, I don't think it makes much of a
> difference, but I don't see why not.
>
> >       } else {
> >               kho_reserve_scratch();
> >       }
>
> --
> Regards,
> Pratyush Yadav