Re: [PATCH] mm/secretmem: properly account locked pages

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clean secretmem folios evicted via `posix_fadvise(POSIX_FADV_DONTNEED)` or `drop_caches` leak `locked_vm` accounting because unaccounting is tied exclusively to inode destruction.
- [High] `secretmem_fault()` evaluates `RLIMIT_MEMLOCK` against the faulting process but charges the `locked_vm` of the file descriptor's creator, enabling limit bypasses and DoS.
--

--- Patch [1]: [PATCH] mm/secretmem: properly account locked pages ---
commit ebb3ab3c96b96ff43e45d7aaef427613df2d6542
Author: Lorenzo Stoakes (ARM) <[email protected]>

mm/secretmem: properly account locked pages

secretmem has a relatively laissez-faire attitude to accounting the folios
it allocates.

The intention is that the memory is treated as if it were mlock()'d and
thus is limited by the RLIMIT_MEMLOCK limit if the CAP_IPC_LOCK capability
is not in place.

[ ... ]

Link: https://lore.kernel.org/linux-mm/[email protected]/

> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index d29865075b6ea..537fe5b1222f2 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -47,10 +49,107 @@ bool secretmem_active(void)
>  	return !!atomic_read(&secretmem_users);
>  }
>  
> +struct secretmem_inode_state {
> +	struct user_struct	*user;
> +	atomic_long_t		nr_pages_accounted;
> +};
> +
> +static bool __secretmem_account_pages(struct user_struct *user,
> +		unsigned long nr_pages)
> +{
> +	unsigned long page_limit, cur_pages, new_pages;
> +
> +	if (!nr_pages)
> +		return true;
> +
> +	page_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> +
> +	cur_pages = atomic_long_read(&user->locked_vm);

[Severity: High]
Does this code check the limit of the faulting process but charge the creator
of the file descriptor?

Looking at __secretmem_account_pages(), it uses rlimit(RLIMIT_MEMLOCK) which
evaluates the limit for the current task, but it increments user->locked_vm
which belongs to the fd creator.

If the fd is passed to a process with a higher or infinite RLIMIT_MEMLOCK,
could the creator bypass their own limit and inflate their locked_vm quota?

Conversely, if the fd is passed to a privileged daemon with a strict
RLIMIT_MEMLOCK, could any page fault by the daemon fail the limit check and
raise a SIGBUS?

> +	do {
> +		new_pages = cur_pages + nr_pages;
> +		if (new_pages > page_limit)
> +			return false;
> +	} while (!atomic_long_try_cmpxchg(&user->locked_vm,
> +					  &cur_pages, new_pages));
> +	return true;
> +}

[ ... ]

> +static void secretmem_destroy_inode_priv(struct inode *inode)
> +{
> +	struct secretmem_inode_state *state = inode->i_private;
> +
> +	if (!state)
> +		return;
> +
> +	secretmem_unaccount_all_folios(state);

[Severity: High]
Could this code leak locked_vm accounting when clean folios are evicted?

If clean secretmem folios are evicted from the page cache via
posix_fadvise(POSIX_FADV_DONTNEED) or drop_caches, they are freed via
secretmem_free_folio().

Since unaccounting is exclusively tied to inode destruction here, are we
missing a decrement to locked_vm when the folio is evicted?

If so, subsequent faults to the same offsets could allocate new folios and
double-charge locked_vm, potentially allowing an unprivileged user to exhaust
the global user_struct->locked_vm quota without actually consuming physical
memory.

> +	free_uid(state->user);
> +	kfree(state);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1