Re: [PATCH 3/9] liveupdate: Make failed retrieves distinguishable from dropped files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Critical] liveupdate: uninitialized file pointer in
  luo_session_retrieve_fd() [1]
- [High] liveupdate: pointer used as XArray index in luo_preserve_file()
  [2]
--

commit acd081aa3a5ef33bac09be5e151d1de15512ea3e
Author: Chris Down <chris@chrisdown.name>

liveupdate: Make failed retrieves distinguishable from dropped files

This commit exposes incoming live update deserialization state and session
and file retrieval counters under /sys/class/misc/liveupdate/incoming/.

> diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
> index a39992da504fb..3aac1afa05c09 100644
> --- a/kernel/liveupdate/luo_file.c
> +++ b/kernel/liveupdate/luo_file.c
> @@ -543,6 +543,16 @@ void luo_file_unfreeze(struct luo_file_set *file_set,
>  	memset(file_set_ser, 0, sizeof(*file_set_ser));
>  }
>  
> +/* Record the first retrieve result. A negative status is permanent. */
> +static void luo_file_set_retrieve_status(struct luo_file *luo_file, int status)
> +{
> +	luo_file->retrieve_status = status;
> +	if (status > 0)
> +		atomic_long_inc(&luo_incoming_stats.files_retrieved);
> +	else
> +		atomic_long_inc(&luo_incoming_stats.files_failed);
> +}
> +
>  /**
>   * luo_retrieve_file - Restores a preserved file from a file_set by its token.
>   * @file_set: The file_set from which to retrieve the file.
> @@ -611,7 +621,7 @@ int luo_retrieve_file(struct luo_file_set *file_set, u64 token,
>  	err = luo_file->fh->ops->retrieve(&args);
>  	if (err) {
>  		/* Keep the error code for later use. */
> -		luo_file->retrieve_status = err;
> +		luo_file_set_retrieve_status(luo_file, err);
>  		return err;
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can a positive return from
fh->ops->retrieve() cause luo_session_retrieve_fd() to pass an uninitialized
struct file pointer to fd_install() [1]?

If fh->ops->retrieve() returns a positive value, luo_retrieve_file() returns
without initializing *filep (and luo_file_set_retrieve_status() records it as
a retrieved file with status > 0).  luo_session_retrieve_fd() only checks
for negative errors before installing the stack variable into the process
file descriptor table:

kernel/liveupdate/luo_session.c:luo_session_retrieve_fd() {
	...
	struct file *file;
	...
	err = luo_retrieve_file(&session->file_set, argp->token, &file);
	mutex_unlock(&session->mutex);
	if (err < 0)
		goto err_put_fd;

	err = luo_ucmd_respond(ucmd, sizeof(*argp));
	if (err)
		goto err_put_file;

	fd_install(argp->fd, file);
}

luo_ioctl_retrieve_session() has the same pattern with luo_session_retrieve().
Any subsequent syscall on the installed fd dereferences the uninitialized
stack pointer.

> @@ -624,7 +634,7 @@ int luo_retrieve_file(struct luo_file_set *file_set, u64 token,
>  			  luo_file->file, GFP_KERNEL));
>  
>  	*filep = luo_file->file;
> -	luo_file->retrieve_status = 1;
> +	luo_file_set_retrieve_status(luo_file, 1);
>  
>  	return 0;
>  }

[Severity: High]
This problem wasn't introduced by this patch, but why does
luo_preserved_files use raw kernel pointers from luo_get_id() as XArray
indices [2]?

kernel/liveupdate/luo_file.c:luo_get_id() {
	return fh->ops->get_id ? fh->ops->get_id(file) : (unsigned long)file;
}

Both luo_preserve_file() and luo_retrieve_file() pass luo_get_id() directly
as the index to xa_insert(&luo_preserved_files, ...).  Because XArray is a
radix tree designed for dense small integer indices, indexing it by sparse
64-bit pointer values forces the allocation of up to 11 levels of
intermediate tree nodes per entry, wasting memory and risking OOM.

[1] https://sashiko.dev/#/bug/linux-65e3446e-27d6-4415-82bb-0036dcc7aa0b
[2] https://sashiko.dev/#/bug/linux-80cdb953-2e71-4aca-9067-108b66f83aaa

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794383544.git.chris@chrisdown.name?part=3