Re: [PATCH v3 3/7] mm, security: Fix missed security_task_movememory()

["Serge E. Hallyn" <serge@hallyn.com>]

On Fri, Dec 01, 2023 at 09:46:32AM +0000, Yafang Shao wrote:
> Considering that MPOL_F_NUMA_BALANCING or  mbind(2) using either
> MPOL_MF_MOVE or MPOL_MF_MOVE_ALL are capable of memory movement, it's
> essential to include security_task_movememory() to cover this
> functionality as well. It was identified during a code review.

Hm - this doesn't have any bad side effects for you when using selinux?
The selinux_task_movememory() hook checks for PROCESS__SETSCHED privs.
The two existing security_task_movememory() calls are in cases where we
expect the caller to be affecting another task identified by pid, so
that makes sense.  Is an MPOL_MV_MOVE to move your own pages actually
analogous to that?

Much like the concern you mentioned in your intro about requiring
CAP_SYS_NICE and thereby expanding its use, it seems that here you
will be regressing some mbind users unless the granting of PROCESS__SETSCHED
is widened.

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  mm/mempolicy.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 10a590ee1c89..1eafe81d782e 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1259,8 +1259,15 @@ static long do_mbind(unsigned long start, unsigned long len,
>  	if (!new)
>  		flags |= MPOL_MF_DISCONTIG_OK;
>  
> -	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> +	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) {

MPOL_MF_MOVE_ALL already has a CAP_SYS_NICE check.  Does that
suffice for that one?

> +		err = security_task_movememory(current);
> +		if (err) {
> +			mpol_put(new);
> +			return err;
> +		}
>  		lru_cache_disable();
> +	}
> +
>  	{
>  		NODEMASK_SCRATCH(scratch);
>  		if (scratch) {
> @@ -1450,6 +1457,8 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode,
>  /* Basic parameter sanity check used by both mbind() and set_mempolicy() */
>  static inline int sanitize_mpol_flags(int *mode, unsigned short *flags)
>  {
> +	int err;
> +
>  	*flags = *mode & MPOL_MODE_FLAGS;
>  	*mode &= ~MPOL_MODE_FLAGS;
>  
> @@ -1460,6 +1469,9 @@ static inline int sanitize_mpol_flags(int *mode, unsigned short *flags)
>  	if (*flags & MPOL_F_NUMA_BALANCING) {
>  		if (*mode != MPOL_BIND)
>  			return -EINVAL;
> +		err = security_task_movememory(current);
> +		if (err)
> +			return err;
>  		*flags |= (MPOL_F_MOF | MPOL_F_MORON);
>  	}
>  	return 0;
> -- 
> 2.30.1 (Apple Git-130)