Re: [PATCH v1 3/7] liveupdate: Pass user argument to file preserve and retrieve

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] core: user-triggerable WARN_ON() crash due to xa_insert allocation failure
--

commit 9f9574a189615ae795011fae912d53ecfd69c013
Author: Pasha Tatashin <pasha.tatashin@soleen.com>

liveupdate: Pass user argument to file preserve and retrieve

This commit allows userspace to pass a file-handler-specific argument structure
when preserving or retrieving an FD via LUO. It extends the preserve and
retrieve structures with a 64-bit arg field, while maintaining compatibility
with older callers. Handlers can now claim files in the preserved files table
before modifying them.

> diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
> index 3c1c86ff14d69..e3bfdbc6baedc 100644
> --- a/kernel/liveupdate/luo_file.c
> +++ b/kernel/liveupdate/luo_file.c
> @@ -176,6 +177,36 @@ static unsigned long luo_get_id(struct liveupdate_file_handler *fh,
>  	return fh->ops->get_id ? fh->ops->get_id(file) : (unsigned long)file;
>  }
>  
> +/**
> + * liveupdate_claim_file - Claim a file in the LUO preserved files table.
> + * @fh:   The file handler claiming @file.
> + * @file: The file to claim.
> + *
> + * File handlers whose .retrieve() callback restores state into an existing
> + * user-supplied &struct file (rather than allocating a new one) should call
> + * this before modifying @file to ensure @file is not already managed by LUO.
> + *
> + * Return: 0 on success, -EBUSY if @file is already managed by LUO, or a
> + *         negative errno on allocation failure.
> + */
> +int liveupdate_claim_file(struct liveupdate_file_handler *fh,
> +			  struct file *file)
> +{
> +	return xa_insert(&luo_preserved_files, luo_get_id(fh, file),
> +			 file, GFP_KERNEL);
> +}

[ ... ]

> @@ -604,9 +687,9 @@ int luo_retrieve_file(struct luo_file_set *file_set, u64 token,
>  	/* Get reference so we can keep this file in LUO until finish */
>  	get_file(luo_file->file);
>  
> -	WARN_ON(xa_insert(&luo_preserved_files,
> -			  luo_get_id(luo_file->fh, luo_file->file),
> -			  luo_file->file, GFP_KERNEL));
> +	if (xa_load(&luo_preserved_files,
> +		    luo_get_id(luo_file->fh, luo_file->file)) != luo_file->file)
> +		WARN_ON(liveupdate_claim_file(luo_file->fh, luo_file->file));

[Severity: Medium]
Is it structurally safe to wrap liveupdate_claim_file() in a WARN_ON() macro?

Because liveupdate_claim_file() uses xa_insert() with GFP_KERNEL, it can
legitimately return -ENOMEM under memory pressure. The WARN_ON() macro will
evaluate this non-zero error code as true and trigger a kernel warning.

Since luo_retrieve_file() is reachable via the LIVEUPDATE_SESSION_RETRIEVE_FD
ioctl, an unprivileged user can trigger this allocation under memory pressure.
On systems with panic_on_warn enabled, this could lead to a user-triggerable
system crash.

>  
>  	*filep = luo_file->file;
>  	luo_file->retrieve_status = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015093816.2438942-1-pasha.tatashin@soleen.com?part=3