Re: [PATCH v5 24/36] mm/mempolicy: add in-kernel MPOL_BIND interfaces for drivers/services

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `mpol_bind_node()` and `mpol_private_bind()` fail to validate `nid` against bounds, leading to stack buffer overflows and out-of-bounds reads.
- [Medium] `__mpol_bind_node()` calls `mpol_set_nodemask()` without holding `task_lock(current)` or `mmap_lock`, causing a data race and potential torn read.
- [Medium] `mpol_bind_node()` unconditionally rejects private node bindings, breaking the documented API contract.
--

--- Patch [24]: [PATCH v5 24/36] mm/mempolicy: add in-kernel MPOL_BIND interfaces for drivers/services ---
commit 056427d8f98ec56375b0a715f69798d4dfde2ec3
Author: Gregory Price <[email protected]>

mm/mempolicy: add in-kernel MPOL_BIND interfaces for drivers/services

This patch adds and exports interfaces to enable modules to build MPOL_BIND
mempolicies pinned to a private NUMA node. It exports mpol_bind_node() and
mpol_private_bind() for module use, widens the __mpol_put() export, and
adjusts mpol_set_nodemask() to handle MPOL_F_PRIVATE flags.

> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index e83c2c7a94c1d..a3ffb09897489 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -432,6 +432,18 @@ static int mpol_set_nodemask(struct mempolicy *pol,
>  	else
>  		pol->w.cpuset_mems_allowed = cpuset_current_mems_allowed;
>  
> +	/*
> +	 * Private nodes are not in cpuset.mems, so they're always stripped.
> +	 * Driver-allocated policies will already have MPOL_F_PRIVATE set,
> +	 * if that's the case, add back in the requested set of private nodes.
> +	 */
> +	for_each_node_mask(nid, *nodes) {
> +		if (!node_is_private(nid))
> +			continue;
> +		if (pol->flags & MPOL_F_PRIVATE)
> +			node_set(nid, nsc->mask2);
> +	}

[Severity: Medium]
The commit message mentions that N_MEMORY_PRIVATE node bindings are honored
if the node has CAP_USER_NUMA. Does this implementation actually perform that
capability check?

It looks like this only checks for MPOL_F_PRIVATE. If mpol_bind_node() calls
__mpol_bind_node() with flags=0, wouldn't the private node be stripped
unconditionally, causing the nodemask to become empty and returning -EINVAL?

[ ... ]

> +static struct mempolicy *__mpol_bind_node(int nid, unsigned short flags)
> +{
> +	struct mempolicy *pol;
> +	nodemask_t nodes;
> +	int err;
> +
> +	NODEMASK_SCRATCH(scratch);
> +
> +	if (!scratch)
> +		return ERR_PTR(-ENOMEM);
> +
> +	nodes_clear(nodes);
> +	node_set(nid, nodes);

[Severity: High]
Should nid be validated against MAX_NUMNODES before being used?

If an invalid or unsanitized node ID like NUMA_NO_NODE (-1) or a value
greater than MAX_NUMNODES is passed from a module, could node_set()
perform an out-of-bounds write on the stack-allocated nodemask_t?

> +
> +	pol = mpol_new(MPOL_BIND, flags, &nodes);
> +	if (IS_ERR(pol)) {
> +		NODEMASK_SCRATCH_FREE(scratch);
> +		return pol;
> +	}
> +
> +	err = mpol_set_nodemask(pol, &nodes, scratch);

[Severity: Medium]
Does this call to mpol_set_nodemask() need to hold task_lock(current) or
mmap_lock?

mpol_set_nodemask() reads cpuset_current_mems_allowed (current->mems_allowed).
Without locking, couldn't this race with concurrent cpuset updates and result
in a torn read or corrupted mempolicy nodemask?

> +	NODEMASK_SCRATCH_FREE(scratch);
> +	if (err) {
> +		mpol_put(pol);
> +		return ERR_PTR(err);
> +	}
> +	return pol;
> +}
> +
> +/**
> + * mpol_private_bind - build an MPOL_BIND policy pinned to a private node
> + * @nid: an N_MEMORY_PRIVATE node
> + *
> + * Returns a refcounted mempolicy that binds allocations to @nid with the
> + * private-placement intent (MPOL_F_PRIVATE).  This binds to @nid regardless
> + * of the node's CAP_USER_NUMA, providing a privileged way for node-owners
> + * to bind driver/service owned VMAs to the node.
> + *
> + * Like any MPOL_BIND it is relaxable: an unsatisfiable request falls back
> + * rather than failing.
> + *
> + * Must be called while @nid is N_MEMORY_PRIVATE.
> + *
> + * The caller owns the reference and frees it with mpol_put().
> + *
> + * Return: the policy, or an ERR_PTR on failure.
> + */
> +struct mempolicy *mpol_private_bind(int nid)
> +{
> +	if (!node_is_private(nid))

[Severity: High]
Similar to the issue in __mpol_bind_node(), if an invalid nid is passed in,
could node_is_private() read out of bounds?

> +		return ERR_PTR(-EINVAL);
> +	return __mpol_bind_node(nid, MPOL_F_PRIVATE);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(mpol_private_bind, "kmem");

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