| 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
| ||
|
Message-ID: <CA+CK2bA5Eyz6TUMTy3pa5HBvZ7KkiHX3EHn17T=d6LX_X5i3bg@mail.gmail.com>
Date: Wed, 15 Oct 2025 08:36:25 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Mike Rapoport <rppt@...nel.org>
Cc: akpm@...ux-foundation.org, brauner@...nel.org, corbet@....net,
graf@...zon.com, jgg@...pe.ca, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-mm@...ck.org, masahiroy@...nel.org,
ojeda@...nel.org, pratyush@...nel.org, rdunlap@...radead.org, tj@...nel.org,
jasonmiu@...gle.com, dmatlack@...gle.com, skhawaja@...gle.com
Subject: Re: [PATCH 1/2] liveupdate: kho: warn and fail on metadata or
preserved memory in scratch area
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@...een.com>
> > ---
> > kernel/liveupdate/Kconfig | 15 ++++++++++
>
> Feels like kernel/liveupdate/Makefile change is missing
It's not, we already have KEXEC_HANDOVER_DEBUGFS that pulls in
kexec_handover_debug.c
That debug file contains KHO debugfs and debug code. The debug code
adds KEXEC_HANDOVER_DEBUGFS as a dependency, which I think is
appropriate for a debug build.
However, I do not like ugly ifdefs in .c, so perhaps, we should have two files:
kexec_handover_debugfs.c for debugfs and kexec_handover_debug.c ? What
do you think?
> > kernel/liveupdate/kexec_handover.c | 32 ++++++++++++++++++---
> > kernel/liveupdate/kexec_handover_debug.c | 18 ++++++++++++
> > kernel/liveupdate/kexec_handover_internal.h | 9 ++++++
> > 4 files changed, 70 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/liveupdate/Kconfig b/kernel/liveupdate/Kconfig
> > index 522b9f74d605..d119f4f3f4b1 100644
> > --- a/kernel/liveupdate/Kconfig
> > +++ b/kernel/liveupdate/Kconfig
> > @@ -27,4 +27,19 @@ config KEXEC_HANDOVER_DEBUGFS
> > Also, enables inspecting the KHO fdt trees with the debugfs binary
> > blobs.
> >
> > +config KEXEC_HANDOVER_DEBUG
> > + bool "Enable Kexec Handover debug checks"
> > + depends on KEXEC_HANDOVER_DEBUGFS
> > + help
> > + This option enables extra sanity checks for the Kexec Handover
> > + subsystem.
> > +
> > + These checks verify that neither preserved memory regions nor KHO's
> > + internal metadata are allocated from within a KHO scratch area.
> > + An overlap can lead to memory corruption during a subsequent kexec
> > + operation.
> > +
> > + If an overlap is detected, the kernel will print a warning and the
> > + offending operation will fail. This should only be enabled for
> > + debugging purposes due to runtime overhead.
> > endmenu
> > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> > index 5da21f1510cc..ef1e6f7a234b 100644
> > --- a/kernel/liveupdate/kexec_handover.c
> > +++ b/kernel/liveupdate/kexec_handover.c
> > @@ -141,6 +141,11 @@ static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t sz)
> > if (!elm)
> > return ERR_PTR(-ENOMEM);
> >
> > + if (WARN_ON(kho_scratch_overlap(virt_to_phys(elm), sz))) {
> > + kfree(elm);
>
> I think __free() cleanup would be better than this.
Sorry, not sure what do you mean. kfree() is already is in this
function in case of failure.
>
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > res = xa_cmpxchg(xa, index, NULL, elm, GFP_KERNEL);
> > if (xa_is_err(res))
> > res = ERR_PTR(xa_err(res));
> > @@ -354,7 +359,13 @@ static struct khoser_mem_chunk *new_chunk(struct khoser_mem_chunk *cur_chunk,
> >
> > chunk = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > if (!chunk)
> > - return NULL;
> > + return ERR_PTR(-ENOMEM);
>
> I don't think it's important to return -errno here, it's not that it's
> called from a syscall and we need to set errno for the userspace.
> BTW, the same applies to xa_load_or_alloc() IMO.
HM, but they are very different errors: ENOMEM, the KHO user can try
again after more memory is available, but the new -EINVAL return from
this function tells the caller that there is something broken in the
system, and using KHO is futile until this bug is fixed.
> > +
> > + if (WARN_ON(kho_scratch_overlap(virt_to_phys(chunk), PAGE_SIZE))) {
> > + kfree(chunk);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > chunk->hdr.order = order;
> > if (cur_chunk)
> > KHOSER_STORE_PTR(cur_chunk->hdr.next, chunk);
> > @@ -379,14 +390,17 @@ static int kho_mem_serialize(struct kho_out *kho_out)
> > struct khoser_mem_chunk *chunk = NULL;
> > struct kho_mem_phys *physxa;
> > unsigned long order;
> > + int ret = -ENOMEM;
> >
> > xa_for_each(&kho_out->track.orders, order, physxa) {
> > struct kho_mem_phys_bits *bits;
> > unsigned long phys;
> >
> > chunk = new_chunk(chunk, order);
> > - if (!chunk)
> > + if (IS_ERR(chunk)) {
> > + ret = PTR_ERR(chunk);
>
> ... and indeed, -errno from new_chunk() juts makes things more complex :(
>
> > goto err_free;
> > + }
> >
> > if (!first_chunk)
> > first_chunk = chunk;
> > @@ -396,8 +410,10 @@ static int kho_mem_serialize(struct kho_out *kho_out)
> >
> > if (chunk->hdr.num_elms == ARRAY_SIZE(chunk->bitmaps)) {
> > chunk = new_chunk(chunk, order);
> > - if (!chunk)
> > + if (IS_ERR(chunk)) {
> > + ret = PTR_ERR(chunk);
> > goto err_free;
> > + }
> > }
> >
> > elm = &chunk->bitmaps[chunk->hdr.num_elms];
> > @@ -414,7 +430,7 @@ static int kho_mem_serialize(struct kho_out *kho_out)
> >
> > err_free:
> > kho_mem_ser_free(first_chunk);
> > - return -ENOMEM;
> > + return ret;
> > }
> >
> > static void __init deserialize_bitmap(unsigned int order,
> > @@ -737,6 +753,9 @@ int kho_preserve_folio(struct folio *folio)
> > const unsigned int order = folio_order(folio);
> > struct kho_mem_track *track = &kho_out.track;
> >
> > + if (WARN_ON(kho_scratch_overlap(pfn << PAGE_SHIFT, PAGE_SIZE << order)))
> > + return -EINVAL;
> > +
> > return __kho_preserve_order(track, pfn, order);
> > }
> > EXPORT_SYMBOL_GPL(kho_preserve_folio);
> > @@ -784,6 +803,11 @@ int kho_preserve_pages(struct page *page, unsigned int nr_pages)
> > unsigned long failed_pfn = 0;
> > int err = 0;
> >
> > + if (WARN_ON(kho_scratch_overlap(start_pfn << PAGE_SHIFT,
> > + nr_pages << PAGE_SHIFT))) {
> > + return -EINVAL;
> > + }
>
> Can't we check this in __kho_preseve_order() and not duplicate the code?
Yes, that is possible, I will move it in the next version.
> > +
> > while (pfn < end_pfn) {
> > const unsigned int order =
> > min(count_trailing_zeros(pfn), ilog2(end_pfn - pfn));
>
> --
> Sincerely yours,
> Mike.
Powered by blists - more mailing lists