split sanitize_mpol_flags into sanitize and validate.
Sanitize is used by set_mempolicy to split (int mode) into mode
and mode_flags, and then validates them.
Validate validates already split flags.
Validate will be reused for new syscalls that accept already
split mode and mode_flags.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
mm/mempolicy.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 0a180c670f0c..59ac0da24f56 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1463,24 +1463,39 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode,
return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0;
}
-/* Basic parameter sanity check used by both mbind() and set_mempolicy() */
-static inline int sanitize_mpol_flags(int *mode, unsigned short *flags)
+/*
+ * Basic parameter sanity check used by mbind/set_mempolicy
+ * May modify flags to include internal flags (e.g. MPOL_F_MOF/F_MORON)
+ */
+static inline int validate_mpol_flags(unsigned short mode, unsigned short *flags)
{
- *flags = *mode & MPOL_MODE_FLAGS;
- *mode &= ~MPOL_MODE_FLAGS;
-
- if ((unsigned int)(*mode) >= MPOL_MAX)
+ if ((unsigned int)(mode) >= MPOL_MAX)
return -EINVAL;
if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES))
return -EINVAL;
if (*flags & MPOL_F_NUMA_BALANCING) {
- if (*mode != MPOL_BIND)
+ if (mode != MPOL_BIND)
return -EINVAL;
*flags |= (MPOL_F_MOF | MPOL_F_MORON);
}
return 0;
}
+/*
+ * Used by mbind/set_memplicy to split and validate mode/flags
+ * set_mempolicy combines (mode | flags), split them out into separate
+ * fields and return just the mode in mode_arg and flags in flags.
+ */
+static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags)
+{
+ unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
+
+ *flags = *mode_arg & MPOL_MODE_FLAGS;
+ *mode_arg = mode;
+
+ return validate_mpol_flags(mode, flags);
+}
+
static long kernel_mbind(unsigned long start, unsigned long len,
unsigned long mode, const unsigned long __user *nmask,
unsigned long maxnode, unsigned int flags)
--
2.39.1Gregory Price <gourry.memverge@gmail.com> writes:
> split sanitize_mpol_flags into sanitize and validate.
>
> Sanitize is used by set_mempolicy to split (int mode) into mode
> and mode_flags, and then validates them.
>
> Validate validates already split flags.
>
> Validate will be reused for new syscalls that accept already
> split mode and mode_flags.
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> mm/mempolicy.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 0a180c670f0c..59ac0da24f56 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1463,24 +1463,39 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode,
> return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0;
> }
>
> -/* Basic parameter sanity check used by both mbind() and set_mempolicy() */
> -static inline int sanitize_mpol_flags(int *mode, unsigned short *flags)
> +/*
> + * Basic parameter sanity check used by mbind/set_mempolicy
> + * May modify flags to include internal flags (e.g. MPOL_F_MOF/F_MORON)
> + */
> +static inline int validate_mpol_flags(unsigned short mode, unsigned short *flags)
> {
> - *flags = *mode & MPOL_MODE_FLAGS;
> - *mode &= ~MPOL_MODE_FLAGS;
> -
> - if ((unsigned int)(*mode) >= MPOL_MAX)
> + if ((unsigned int)(mode) >= MPOL_MAX)
> return -EINVAL;
> if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES))
> return -EINVAL;
> if (*flags & MPOL_F_NUMA_BALANCING) {
> - if (*mode != MPOL_BIND)
> + if (mode != MPOL_BIND)
> return -EINVAL;
> *flags |= (MPOL_F_MOF | MPOL_F_MORON);
> }
> return 0;
> }
>
> +/*
> + * Used by mbind/set_memplicy to split and validate mode/flags
> + * set_mempolicy combines (mode | flags), split them out into separate
mbind() uses mode flags too.
> + * fields and return just the mode in mode_arg and flags in flags.
> + */
> +static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags)
> +{
> + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
> +
> + *flags = *mode_arg & MPOL_MODE_FLAGS;
> + *mode_arg = mode;
It appears that it's unnecessary to introduce a local variable to split
mode/flags. Just reuse the original code?
> +
> + return validate_mpol_flags(mode, flags);
> +}
> +
> static long kernel_mbind(unsigned long start, unsigned long len,
> unsigned long mode, const unsigned long __user *nmask,
> unsigned long maxnode, unsigned int flags)
--
Best Regards,
Huang, Ying
On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > + * fields and return just the mode in mode_arg and flags in flags.
> > + */
> > +static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags)
> > +{
> > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
> > +
> > + *flags = *mode_arg & MPOL_MODE_FLAGS;
> > + *mode_arg = mode;
>
> It appears that it's unnecessary to introduce a local variable to split
> mode/flags. Just reuse the original code?
>
ack
~Gregory
On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote: > On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote: > > Gregory Price <gourry.memverge@gmail.com> writes: > > > > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); > > > + > > > + *flags = *mode_arg & MPOL_MODE_FLAGS; > > > + *mode_arg = mode; > > > > It appears that it's unnecessary to introduce a local variable to split > > mode/flags. Just reuse the original code? > > Revisiting during fixes: Note the change from int to short. I chose to make this explicit because validate_mpol_flags takes a short. I'm fairly sure changing it back throws a truncation warning. ~Gregroy
Gregory Price <gregory.price@memverge.com> writes:
> On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote:
>> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote:
>> > Gregory Price <gourry.memverge@gmail.com> writes:
>> >
>> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
>> > > +
>> > > + *flags = *mode_arg & MPOL_MODE_FLAGS;
>> > > + *mode_arg = mode;
>> >
>> > It appears that it's unnecessary to introduce a local variable to split
>> > mode/flags. Just reuse the original code?
>> >
>
> Revisiting during fixes: Note the change from int to short.
>
> I chose to make this explicit because validate_mpol_flags takes a short.
>
> I'm fairly sure changing it back throws a truncation warning.
Why something like below doesn't work?
int sanitize_mpol_flags(int *mode, unsigned short *flags)
{
*flags = *mode & MPOL_MODE_FLAGS;
*mode &= ~MPOL_MODE_FLAGS;
return validate_mpol_flags(*mode, flags);
}
--
Best Regards,
Huang, Ying
On Tue, Jan 02, 2024 at 05:09:55PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> > On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote:
> >> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote:
> >> > Gregory Price <gourry.memverge@gmail.com> writes:
> >> >
> >> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS);
> >> > > +
> >> > > + *flags = *mode_arg & MPOL_MODE_FLAGS;
> >> > > + *mode_arg = mode;
> >> >
> >> > It appears that it's unnecessary to introduce a local variable to split
> >> > mode/flags. Just reuse the original code?
> >> >
> >
> > Revisiting during fixes: Note the change from int to short.
> >
> > I chose to make this explicit because validate_mpol_flags takes a short.
> >
> > I'm fairly sure changing it back throws a truncation warning.
>
> Why something like below doesn't work?
>
> int sanitize_mpol_flags(int *mode, unsigned short *flags)
> {
> *flags = *mode & MPOL_MODE_FLAGS;
> *mode &= ~MPOL_MODE_FLAGS;
>
> return validate_mpol_flags(*mode, flags);
> }
was concerned with silent truncation of (*mode) (int) to short.
*shrug* happy to change it
~Gregory
© 2016 - 2025 Red Hat, Inc.