Re: [RFC PATCH v2 07/10] kvm: guest_memfd_luo: add support for guest_memfd preservation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Dereferencing `file->private_data` before validating the file type causes a NULL pointer dereference.
- [High] Logic inversion in `kvm_gmem_luo_finish()` causes a double-free of preservation structures and active guest memory folios on successful retrieval.
- [High] Missing `kho_is_enabled()` gate before invoking KHO serialization APIs causes potential NULL pointer dereference.
--

commit d0ebe234914d6981be774d511af45334f3db4977
Author: Tarun Sahu <tarunsahu@google.com>
Subject: kvm: guest_memfd_luo: add support for guest_memfd preservation

This patch sets up the basic infrastructure to preserve the guest_memfd.
It registers a new LUO file handler for guest_memfd files to serialize
and deserialize guest memory.

> diff --git a/virt/kvm/guest_memfd_luo.c b/virt/kvm/guest_memfd_luo.c
> new file mode 100644
> index 0000000000000..d466f889c9aa1
> --- /dev/null
> +++ b/virt/kvm/guest_memfd_luo.c
> @@ -0,0 +1,485 @@
[ ... ]
> +static bool kvm_gmem_luo_can_preserve(struct liveupdate_file_handler *handler, struct file *file)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct gmem_file *gmem_file = file->private_data;
> +	struct kvm *kvm = gmem_file->kvm;

[Severity: Critical]
Does this code dereference file->private_data before confirming the file
is actually a guest_memfd?

The LUO framework might pass unrelated file descriptors where
private_data is NULL. By extracting gmem_file->kvm before checking
inode->i_sb->s_magic, could this cause a NULL pointer dereference
when evaluating unrelated files?

> +
> +	if (inode->i_sb->s_magic != GUEST_MEMFD_MAGIC)
> +		return 0;
> +
[ ... ]
> +static int kvm_gmem_luo_preserve(struct liveupdate_file_op_args *args)
> +{
[ ... ]
> +	end_index = size >> PAGE_SHIFT;
> +
> +	ser = kho_alloc_preserve(sizeof(*ser));

[Severity: High]
Could this call trigger a NULL pointer dereference if the KHO subsystem
is disabled?

According to the KHO subsystem guidelines, all callers must explicitly
gate KHO usage on kho_is_enabled(). It looks like kvm_gmem_luo_init()
registers the file handler unconditionally, and this path invokes
kho_alloc_preserve() and other KHO serialization APIs without checking.

Should this function (or the initialization) verify kho_is_enabled()
before interacting with the KHO subsystem?

> +	if (IS_ERR(ser)) {
> +		err = PTR_ERR(ser);
> +		goto err_unfreeze_inode;
> +	}
[ ... ]
> +static int kvm_gmem_luo_retrieve(struct liveupdate_file_op_args *args)
> +{
[ ... ]
> +	args->file = file;
> +	kho_restore_free(ser);
> +	return 0;
> +
> +err_put_remaining_folios:
[ ... ]
> +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;

[Severity: High]
Does this logic inversion cause a double-free on successful retrieval?

In kvm_gmem_luo_retrieve(), a successful retrieval frees the ser struct
but leaves args->serialized_data intact:

kvm_gmem_luo_retrieve()
    kho_restore_free(ser);
    return 0;

Because standard Linux error handling uses 0 for success,
if (args->retrieve_status) evaluates to false on success, causing this
function to fall through instead of returning.

Could this lead to accessing the freed ser struct, erroneously putting
active folios, and calling kho_restore_free() a second time?

> +
> +	if (!args->serialized_data)
> +		return;
> +
> +	ser = phys_to_virt(args->serialized_data);
> +	if (!ser)
> +		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780676742.git.tarunsahu@google.com?part=7