Re: [PATCH] mm/memfd_luo: carry the exec-bit state in the preserved record

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: TOCTOU race in memfd_luo_preserve loses fchmod and seals updates
- [Medium] mm/memfd_luo: leak of preserved folios in memfd_luo_retrieve() error paths
--

commit b09cba94dd95aadc45f383b05eb9e6f205bf88e7
Author: Haolong Wang <18766350203@163.com>
Subject: mm/memfd_luo: carry the exec-bit state in the preserved record

This commit preserves the execution bits of a memfd during a KHO live
update. It records the exec-bit state in the memfd_luo_ser structure and
applies it to the fresh inode during restore before re-applying seals,
ensuring the W^X policy behaves correctly.

> diff --git a/mm/memfd_luo.c b/mm/memfd_luo.c
> --- a/mm/memfd_luo.c
> +++ b/mm/memfd_luo.c
> @@ -299,6 +299,17 @@ static int memfd_luo_preserve(struct liveupdate_file_op_args *args)
>  	ser->size = inode_size;
>  	ser->seals = seals;
>  
> +	/*
> +	 * F_SEAL_EXEC freezes the exec bits as they are at sealing time, so
> +	 * the restore has to reproduce them.  Record whether the inode had
> +	 * exec bits here; the restore sets the mode of the fresh inode
> +	 * accordingly before re-applying the seals.  ser is zero-initialized,
> +	 * so a clear bit means the bits were clear (or the record predates
> +	 * this flag -- the safe reading for those is also "no exec bits").
> +	 */
> +	if (inode->i_mode & 0111)
> +		ser->flags |= MEMFD_LUO_FL_EXEC_BIT;
> +

[Severity: High]
Can a concurrent userspace modification race with this early caching?

The i_mode is read and cached here, before dropping the inode_lock. Shortly
after, shmem_freeze() is called. However, SHMEM_F_MAPPING_FROZEN ignores
ATTR_MODE in shmem_setattr() and is not checked in memfd_add_seals().

Because memfd_luo_freeze() blindly updates only f_pos without re-evaluating
the mode or seals:

mm/memfd_luo.c:memfd_luo_freeze() {
    ...
    ser->pos = args->file->f_pos;
    ...
}

Could userspace modify the mode via fchmod or add seals between the preserve
and freeze phases, causing the restore process to lose those updates and
break security policies like the W^X rule?

[ ... ]

> @@ -542,6 +556,19 @@ static int memfd_luo_retrieve(struct liveupdate_file_op_args *args)
>  		goto free_ser;
>  	}
>  
> +	/*
> +	 * Reproduce the exec bits before the seals are re-applied: F_SEAL_EXEC
> +	 * freezes the bits as they were at sealing time, and the W^X rule in
> +	 * memfd_add_seals() looks at the mode when it decides whether to imply
> +	 * the write seals.  Setting the mode here, directly on the fresh
> +	 * anonymous inode, is safe -- the file has not been published and no
> +	 * other thread can reach it yet.
> +	 */
> +	if (ser->flags & MEMFD_LUO_FL_EXEC_BIT)
> +		file->f_inode->i_mode |= 0111;
> +	else
> +		file->f_inode->i_mode &= ~0111;
> +
>  	err = memfd_add_seals(file, ser->seals);
>  	if (err) {
>  		pr_err("failed to add seals: %pe\n", ERR_PTR(err));
>  		goto put_file;
>  	}

[Severity: Medium]
Do these error paths leak the physical folios tracked by ser->folios?

When jumping to free_ser or put_file on failure, kho_restore_free(ser) is
called, but there is no call to restore the vmalloc metadata via
kho_restore_vmalloc() and free the physical folios.

Looking at memfd_luo_finish(), the driver explicitly expects retrieve() to
clean up its own folios on failure:

mm/memfd_luo.c:memfd_luo_finish() {
    /*
     * If retrieve was successful, nothing to do. If it failed, retrieve()
     * already cleaned up everything it could. So nothing to do there
     * either. Only need to clean up when retrieve was not called.
     */
    if (args->retrieve_status)
        return;
    ...
}

Will failing at memfd_alloc_file() or memfd_add_seals() leave these
KHO-preserved folios permanently reserved in memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014003558.357783-1-18766350203@163.com?part=1