| 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+CK2bDkc9POgvZpHFWzTsbic6UjExU+YN5SO=D-ODaHQRMTFQ@mail.gmail.com>
Date: Sat, 18 Oct 2025 11:31:37 -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
On Thu, Oct 16, 2025 at 1:23 PM Mike Rapoport <rppt@...nel.org> wrote:
>
> On Wed, Oct 15, 2025 at 08:36:25AM -0400, Pasha Tatashin wrote:
> > > > 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.
>
> There's __free(kfree) cleanup function defined in include/linux/cleanup.h
> that ensures that on return from a function resources are not leaked.
> With kfree we could do something like
>
> void *elm __free(kfree) = NULL;
>
> if (error)
> return ERR_PTR(errno);
>
> return no_free_ptr(elm);
>
> There's no __free() definition for free_page() though :(
>
> The second best IMHO is to use goto for error handling rather than free()
> inside if (error).
Makes sense, let me try to get it done. I actually like what cleanup.h provides.
>
> > > > + 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.
>
> Do you really see the callers handling this differently?
> And we already have WARN_ON() because something is broken in the system.
Maybe, also maybe for some self-tests. I think it is more consistent
to return PTR_ERR() based on other code in this file (i.e.
xa_load_or_alloc() already uses it). Let's keep it.
>
> > > > +
> > > > + if (WARN_ON(kho_scratch_overlap(virt_to_phys(chunk), PAGE_SIZE))) {
> > > > + kfree(chunk);
> > > > + return ERR_PTR(-EINVAL);
> > > > + }
> > > > +
>
> --
> Sincerely yours,
> Mike.
Powered by blists - more mailing lists