Re: [RFC PATCH v2 4/4] mm: memfd_luo: Add the 2-bit pair feature header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: memfd_luo: missing feature header validation in memfd_luo_finish()
--

commit b960724786ac11078fdfa9ded6bcd2355170cf0b
Author: Tarun Sahu <tarunsahu@google.com>

mm: memfd_luo: Add the 2-bit pair feature header

This commit embeds a 128-byte struct luo_feature_hdr at the start of struct
memfd_luo_ser and defines enum memfd_luo_feature with MEMFD_LUO_FEAT_CORE as
a required feature. It initializes the feature header during
memfd_luo_preserve() and validates it during memfd_luo_retrieve().

> diff --git a/mm/memfd_luo.c b/mm/memfd_luo.c
> index 64f8b8cb75bdb..ea1f4c8cd2591 100644
> --- a/mm/memfd_luo.c
> +++ b/mm/memfd_luo.c

[ ... ]

> @@ -525,6 +531,14 @@ static int memfd_luo_retrieve(struct liveupdate_file_op_args *args)
>  	if (!ser)
>  		return -EINVAL;
>  
> +	err = luo_feature_hdr_validate(&ser->features, memfd_luo_features);
> +	if (err) {
> +		pr_err("memfd_luo_ser feature header validation failed (size=%llu, flags[0]=%#llx): %pe\n",
> +		       ser->features.size, ser->features.flags[0],
> +		       ERR_PTR(err));
> +		goto free_ser;
> +	}
> +
>  	/* Make sure the file only has seals supported by this version. */
>  	if (ser->seals & ~MEMFD_LUO_ALL_SEALS) {
>  		err = -EOPNOTSUPP;

[Severity: Medium]
Here in memfd_luo_retrieve(), ser->features is validated via
luo_feature_hdr_validate() before accessing any MEMFD_LUO_FEAT_CORE fields,
jumping directly to kho_restore_free(ser) on failure.

However, when a preserved memfd is not retrieved in the target kernel before
the session is finished or closed (for example via luo_session_release() ->
luo_session_finish_one() -> luo_file_finish() -> luo_file_finish_one() ->
memfd_luo_finish() with args->retrieve_status == 0), memfd_luo_finish()
reads ser->nr_folios and ser->folios without validating ser->features first:

mm/memfd_luo.c:memfd_luo_finish() {
	if (args->retrieve_status)
		return;

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

	if (ser->nr_folios) {
		folios_ser = kho_restore_vmalloc(&ser->folios);
		if (!folios_ser)
			goto out;

		memfd_luo_discard_folios(folios_ser, ser->nr_folios);
		vfree(folios_ser);
	}

out:
	kho_restore_free(ser);
}

According to the ABI contract in include/linux/kho/abi/memfd.h, fields of a
LUO_FEAT_REQUIRED feature such as nr_folios and folios are only guaranteed
present once luo_feature_hdr_validate() succeeds.

Should memfd_luo_finish() also call luo_feature_hdr_validate(&ser->features,
memfd_luo_features) and skip to kho_restore_free(ser) if validation fails,
so it does not pass unvalidated MEMFD_LUO_FEAT_CORE fields to
kho_restore_vmalloc() and memfd_luo_discard_folios()?

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