Re: [PATCH v1 6/7] mm/memfd_luo: Support tmpfs preservation and retrieval

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: memfd_luo_retrieve drops file seals on tmpfs retrieval causing security bypass
- [High] mm: memfd_luo_retrieve ABBA deadlock between inode lock and f_pos_lock
- [High] mm: permanent leak of KHO folios on early memfd_luo_retrieve errors
--

commit 54190d07831630c2e724ceea33310f62bf42a6dd
Author: David Matlack <dmatlack@google.com>

mm/memfd_luo: Support tmpfs preservation and retrieval

This commit relaxes memfd_luo_can_preserve() to support general shmem-backed
files, allowing userspace to preserve standard tmpfs files across kexec. It
introduces a new MEMFD_LUO_FLAG_TMPFS flag and a new UAPI struct
liveupdate_memfd_retrieve_arg to support restoring preserved tmpfs folios
to a newly created tmpfs file.

> diff --git a/mm/memfd_luo.c b/mm/memfd_luo.c
> --- a/mm/memfd_luo.c
> +++ b/mm/memfd_luo.c
> @@ -516,10 +519,60 @@ static int memfd_luo_retrieve_folios(struct file *file,
>  	return err;
>  }
>  
> +static struct file *memfd_luo_get_tmpfs_file(struct liveupdate_file_op_args *args)
> +{
> +	const struct liveupdate_memfd_retrieve_arg *rarg = args->arg;

[ ... ]

> +	inode = file_inode(file);
> +	inode_lock(inode);
> +	if (!inode->i_nlink) {
> +		err = -EINVAL;
> +		goto unlock_inode;
> +	}

[ ... ]

> +	return file;
> +
> +unlock_inode:
> +	inode_unlock(inode);
> +put_file:
> +	fput(file);
> +	return ERR_PTR(err);
> +}
> +
>  static int memfd_luo_retrieve(struct liveupdate_file_op_args *args)
>  {
>  	struct memfd_luo_folio_ser *folios_ser;
>  	struct memfd_luo_ser *ser;
> +	struct inode *inode;
>  	struct file *file;
>  	int err;
>  
> @@ -527,50 +580,75 @@ static int memfd_luo_retrieve(struct liveupdate_file_op_args *args)
>  	if (!ser)
>  		return -EINVAL;
>  
> -	/* Make sure the file only has seals supported by this version. */
> -	if (ser->seals & ~MEMFD_LUO_ALL_SEALS) {
> +	/* Make sure the file only has seals and flags supported by this version. */
> +	if ((ser->seals & ~MEMFD_LUO_ALL_SEALS) ||
> +	    (ser->flags & ~MEMFD_LUO_ALL_FLAGS)) {
>  		err = -EOPNOTSUPP;
>  		goto free_ser;
>  	}

[Severity: High]
Does this error path permanently leak the KHO memory?

If memfd_luo_retrieve() encounters a permanent error here (like unsupported
seals), it jumps to free_ser and returns an error without setting
args->retryable = true. The LUO core then records this negative error code
in luo_file->retrieve_status. 

When memfd_luo_finish() is later called:

	if (args->retrieve_status)
		return;

It returns early because args->retrieve_status != 0, never attempting to
discard the ser->folios vmalloc array or the underlying physical data
folios. Those folios would remain permanently reserved in memblock.

> +	if (ser->flags & MEMFD_LUO_FLAG_TMPFS) {
> +		file = memfd_luo_get_tmpfs_file(args);
> +		if (IS_ERR(file)) {
> +			args->retryable = true;
> +			return PTR_ERR(file);
> +		}
> +		inode = file_inode(file);
> +	} else {
> +		if (args->arg) {
> +			args->retryable = true;
> +			return -EINVAL;
> +		}
>  
> -		/*
> -		 * The seals are preserved. Allow sealing here so they can be added
> -		 * later.
> -		 */
> -		file = memfd_alloc_file("", MFD_ALLOW_SEALING);
> -		if (IS_ERR(file)) {
> -			pr_err("failed to setup file: %pe\n", file);
> -			err = PTR_ERR(file);
> -			goto free_ser;
> -		}
> +		/*
> +		 * The seals are preserved. Allow sealing here so they can be
> +		 * added later.
> +		 */
> +		file = memfd_alloc_file("", MFD_ALLOW_SEALING);
> +		if (IS_ERR(file)) {
> +			pr_err("failed to setup file: %pe\n", file);
> +			err = PTR_ERR(file);
> +			goto free_ser;
> +		}
>  
> -	err = memfd_add_seals(file, ser->seals);
> -	if (err) {
> -		pr_err("failed to add seals: %pe\n", ERR_PTR(err));
> -		goto put_file;
> -	}
> +		err = memfd_add_seals(file, ser->seals);
> +		if (err) {
> +			pr_err("failed to add seals: %pe\n", ERR_PTR(err));
> +			goto put_file;
> +		}
> +
> +		inode = file_inode(file);
> +		inode_lock(inode);
> +	}

[Severity: High]
Does this bypass security seals on retrieved tmpfs files?

memfd_luo_preserve() records seals in ser->seals for all files. However,
the tmpfs branch above entirely skips calling memfd_add_seals(). If an
immutable or write-sealed memfd was linked to the filesystem prior to kexec,
it appears it will be restored into the user's empty tmpfs file without any
seals applied, resulting in a fully writable and modifiable file.

>  	if (ser->nr_folios) {
>  		folios_ser = kho_restore_vmalloc(&ser->folios);
>  		if (!folios_ser) {
>  			err = -EINVAL;
> -			goto put_file;
> +			goto unclaim_file;
>  		}
>  
>  		err = memfd_luo_retrieve_folios(file, folios_ser, ser->nr_folios);
>  		vfree(folios_ser);
>  		if (err)
> -			goto put_file;
> +			goto unclaim_file;
>  	}
>  
> +	vfs_setpos(file, ser->pos, MAX_LFS_FILESIZE);

[Severity: High]
Can this create an ABBA deadlock between inode_lock and f_pos_lock?

For tmpfs files, memfd_luo_get_tmpfs_file() acquires and returns with
inode_lock(inode) held. Later, vfs_setpos() is called here, which acquires
mutex_lock(&file->f_pos_lock) while inode_lock is still held.

Since the file is a normal file descriptor passed by userspace, the standard
VFS write path acquires file->f_pos_lock first via fdget_pos(), and then
acquires inode_lock(inode) inside the filesystem's write iterator. A
concurrent write() on the provided file descriptor could trigger a deadlock.

> +	i_size_write(inode, ser->size);
> +	inode_unlock(inode);
> +
>  	args->file = file;
>  	kho_restore_free(ser);
>  
>  	return 0;

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