Re: [PATCH v4 08/11] KVM: guest_memfd: Add support for preservation via LUO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `kvm_gmem_luo_finish` incorrectly performs cleanup on successful retrieval, resulting in use-after-free and double-frees of the serialization state and premature freeing of page cache folios.
- [High] Missing `kho_is_enabled()` check before invoking KHO serialization APIs, risking a NULL pointer dereference.
--

--- Patch [8]: [PATCH v4 08/11] KVM: guest_memfd: Add support for preservation via LUO ---
commit 8d81cd3b20bca211fdf1860f352a2bbcf1096ec9
Author: Tarun Sahu <tarunsahu@google.com>

KVM: guest_memfd: Add support for preservation via LUO

This commit registers a Live Update Orchestrator file handler for
guest_memfd files to preserve guest memory across kexec reboots. It freezes
the guest_memfd inode during preservation, walks the page cache to serialize
metadata, and reconstructs the file and its folios on retrieval.

> diff --git a/virt/kvm/guest_memfd_luo.c b/virt/kvm/guest_memfd_luo.c
> new file mode 100644
> index 0000000000000..c0aa9354e0a78
> --- /dev/null
> +++ b/virt/kvm/guest_memfd_luo.c
> @@ -0,0 +1,515 @@
[ ... ]
> +static int kvm_gmem_luo_preserve(struct liveupdate_file_op_args *args)
> +{
> +	struct guest_memfd_luo_folio_ser *folios_ser = NULL;
> +	u64 count = 0, gmem_flags, abi_flags = 0;
> +	struct guest_memfd_luo_ser *ser;
[ ... ]
> +	end_index = size >> PAGE_SHIFT;
> +
> +	ser = kho_alloc_preserve(sizeof(*ser));

[Severity: High]
Does this path need to verify if the KHO subsystem is actually enabled?

If the kernel is booted with KHO disabled, calling kho_alloc_preserve() and
other KHO APIs unconditionally can cause a NULL pointer dereference. 

Could we check kho_is_enabled() either here or in kvm_gmem_luo_init() before
registering the handler?

[ ... ]
> +static void kvm_gmem_luo_finish(struct liveupdate_file_op_args *args)
> +{
> +	struct guest_memfd_luo_ser *ser;
> +	struct guest_memfd_luo_folio_ser *folios_ser;
> +
> +	/* Nothing to be done here, if retrieve_status was successful or errored,
> +	 * Cleanup is taken care of in retrieval call.
> +	 */
> +	if (args->retrieve_status)
> +		return;
> +
> +	if (!args->serialized_data)
> +		return;
> +
> +	ser = phys_to_virt(args->serialized_data);

[Severity: High]
Is the check on args->retrieve_status inverted here?

When kvm_gmem_luo_retrieve() succeeds, it returns 0 and frees the ser pointer:

kvm_gmem_luo_retrieve()
    args->file = file;
    kho_restore_free(ser);
    return 0;

Because retrieve_status is 0 on success, this finish function will fall
through the if statement instead of returning early. It will then access
the freed ser pointer and mistakenly discard all the successfully restored
folios.

Would checking if (!args->retrieve_status) avoid this use-after-free
regression?

> +
> +	if (ser->nr_folios) {
> +		folios_ser = kho_restore_vmalloc(&ser->folios);
> +		if (folios_ser) {
> +			kvm_gmem_luo_discard_folios(folios_ser, ser->nr_folios, 0);
> +			vfree(folios_ser);
> +		}
> +	}
> +
> +	kho_restore_free(ser);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728121138.1103610-1-tarunsahu@google.com?part=8