[PATCH 0/3] ipc: Clamp msgmni and shmmni to the real IPC_MNI limit

[PATCH 0/3] ipc: Clamp msgmni and shmmni to the real IPC_MNI limit

[Waiman Long]

The sysctl parameters msgmni and shmmni have an inherent limit of
IPC_MNI (32k). However, users may not be aware of that because they
can write a value much higher than that without getting any error or
notification. Reading the parameters back will show the newly written
values which are not real.

Enforcing the limit by failing sysctl parameter write, however,
can break existing user applications. To address this delemma, the
following new sysctl range clamping APIs are added:
  - proc_dointvec_clamp_minmax()
  - proc_douintvec_clamp_minmax()

This new set of APIs is then used to set the real limit for msgmni and
shmmni without breaking existing applications. If any out of range
value is written to those sysctl parameters, the following warning
will be printed instead.

  Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.

Reading the values back will show 32768 instead of some fake values.

Waiman Long (3):
  sysctl: Add range clamping intvec helper functions
  sysctl: Warn when a clamped sysctl parameter is set out of range
  ipc: Clamp msgmni and shmmni to the real IPC_MNI limit

 include/linux/sysctl.h |   6 +++
 ipc/ipc_sysctl.c       |  10 ++--
 kernel/sysctl.c        | 123 ++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 129 insertions(+), 10 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] sysctl: Add range clamping intvec helper functions

[Waiman Long]

The current intvec range helper functions will fail with error when
users try to assign an out-of-range value.

The following new non-failing range helper functions are now added:
 - proc_dointvec_clamp_minmax()
 - proc_douintvec_clamp_minmax()

The new helper functions will clamp the value to within the given
min/max range without failing it.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/sysctl.h |  6 ++++
 kernel/sysctl.c        | 97 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 96 insertions(+), 7 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index b769ecf..7684ea5 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -63,6 +63,12 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
 				      void __user *, size_t *, loff_t *);
 extern int proc_do_large_bitmap(struct ctl_table *, int,
 				void __user *, size_t *, loff_t *);
+extern int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
+				      void __user *buffer, size_t *lenp,
+				      loff_t *ppos);
+extern int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
+				       void __user *buffer, size_t *lenp,
+				       loff_t *ppos);
 
 /*
  * Register a set of sysctl names by calling register_sysctl_table
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f98f28c..f86c3a7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2500,9 +2500,15 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 }
 #endif
 
+/*
+ * The clamping flag, if set, will clamp the input value to the range
+ * specified by the given min/max values instead of returning error when
+ * out of range.
+ */
 struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
+	bool clamp;
 };
 
 static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
@@ -2512,9 +2518,18 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
-			return -EINVAL;
+		if (param->min && *param->min > val) {
+			if (param->clamp)
+				val = *param->min;
+			else
+				return -EINVAL;
+		}
+		if (param->max && *param->max < val) {
+			if (param->clamp)
+				val = *param->max;
+			else
+				return -EINVAL;
+		}
 		*valp = val;
 	} else {
 		int val = *valp;
@@ -2556,9 +2571,38 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
+/**
+ * proc_dointvec_clamp_minmax - read a vector of integers with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) integer
+ * values from/to the user buffer, treated as an ASCII string.
+ *
+ * This routine will clamp the values to within the range specified by
+ * table->extra1 (min) and table->extra2 (max).
+ *
+ * Returns 0 on success.
+ */
+int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
+		  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct do_proc_dointvec_minmax_conv_param param = {
+		.min = (int *) table->extra1,
+		.max = (int *) table->extra2,
+		.clamp = true,
+	};
+	return do_proc_dointvec(table, write, buffer, lenp, ppos,
+				do_proc_dointvec_minmax_conv, &param);
+}
+
 struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
+	bool clamp;
 };
 
 static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
@@ -2573,10 +2617,18 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 		if (*lvalp > UINT_MAX)
 			return -EINVAL;
 
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
-			return -ERANGE;
-
+		if (param->min && *param->min > val) {
+			if (param->clamp)
+				val = *param->min;
+			else
+				return -ERANGE;
+		}
+		if (param->max && *param->max < val) {
+			if (param->clamp)
+				val = *param->max;
+			else
+				return -ERANGE;
+		}
 		*valp = val;
 	} else {
 		unsigned int val = *valp;
@@ -2616,6 +2668,37 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 				 do_proc_douintvec_minmax_conv, &param);
 }
 
+/**
+ * proc_douintvec_clamp_minmax - read a vector of uints with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
+ * values from/to the user buffer, treated as an ASCII string. Negative
+ * strings are not allowed.
+ *
+ * This routine will clamp the values to within the range specified by
+ * table->extra1 (min) and table->extra2 (max). There is a final sanity
+ * check for UINT_MAX to avoid having to support wrap around uses from
+ * userspace.
+ *
+ * Returns 0 on success.
+ */
+int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
+			  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct do_proc_douintvec_minmax_conv_param param = {
+		.min = (unsigned int *) table->extra1,
+		.max = (unsigned int *) table->extra2,
+		.clamp = true,
+	};
+	return do_proc_douintvec(table, write, buffer, lenp, ppos,
+				 do_proc_douintvec_minmax_conv, &param);
+}
+
 static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 					unsigned int *valp,
 					int write, void *data)
-- 
1.8.3.1

[PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

Even with clamped sysctl parameters, it is still not that straight
forward to figure out the exact range of those parameters. One may
try to write extreme parameter values to see if they get clamped.
To make it easier, a warning with the expected range will now be
printed in the kernel ring buffer when a clamped sysctl parameter
receives an out of range value.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/sysctl.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f86c3a7..acdf4e8 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2508,6 +2508,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
+	const char *name;
 	bool clamp;
 };
 
@@ -2516,21 +2517,33 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int write, void *data)
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
+		bool clamped = false;
+
 		if (param->min && *param->min > val) {
-			if (param->clamp)
+			if (param->clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (param->clamp)
+			if (param->clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name)
+			pr_warn("Kernel parameter \"%s\" was set out of range [%d, %d], clamped to %d.\n",
+				param->name,
+				param->min ? *param->min : -INT_MAX,
+				param->max ? *param->max :  INT_MAX, val);
 	} else {
 		int val = *valp;
 		if (val < 0) {
@@ -2593,6 +2606,7 @@ int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
 	struct do_proc_dointvec_minmax_conv_param param = {
 		.min = (int *) table->extra1,
 		.max = (int *) table->extra2,
+		.name = table->procname,
 		.clamp = true,
 	};
 	return do_proc_dointvec(table, write, buffer, lenp, ppos,
@@ -2602,6 +2616,7 @@ int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
 struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
+	const char *name;
 	bool clamp;
 };
 
@@ -2613,23 +2628,33 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 
 	if (write) {
 		unsigned int val = *lvalp;
+		bool clamped = false;
 
 		if (*lvalp > UINT_MAX)
 			return -EINVAL;
 
 		if (param->min && *param->min > val) {
-			if (param->clamp)
+			if (param->clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (param->clamp)
+			if (param->clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name)
+			pr_warn("Kernel parameter \"%s\" was set out of range [%u, %u], clamped to %u.\n",
+				param->name,
+				param->min ? *param->min : 0,
+				param->max ? *param->max : UINT_MAX, val);
 	} else {
 		unsigned int val = *valp;
 		*lvalp = (unsigned long) val;
@@ -2693,6 +2718,7 @@ int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
 	struct do_proc_douintvec_minmax_conv_param param = {
 		.min = (unsigned int *) table->extra1,
 		.max = (unsigned int *) table->extra2,
+		.name = table->procname,
 		.clamp = true,
 	};
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
-- 
1.8.3.1

[PATCH 3/3] ipc: Clamp msgmni and shmmni to the real IPC_MNI limit

[Waiman Long]

A user can write arbitrary integer values to msgmni and shmmni sysctl
parameters without getting error, but the actual limit is really
IPC_MNI (32k). This can mislead users as they think they can get a
value that is not real.

Enforcing the limit by failing the sysctl parameter write, however,
can break existing user applications. Instead, the clamping version
of the sysctl parameter range checking API is used to enforce the
limit without failing existing user code. Users can easily figure out
if the sysctl parameter value is out of range by either reading back
the parameter value or checking the kernel ring buffer for warning.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 ipc/ipc_sysctl.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 8ad93c2..03abcdd 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -46,7 +46,8 @@ static int proc_ipc_dointvec_minmax(struct ctl_table *table, int write,
 	memcpy(&ipc_table, table, sizeof(ipc_table));
 	ipc_table.data = get_ipc(table);
 
-	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+	return proc_dointvec_clamp_minmax(&ipc_table, write, buffer, lenp,
+					  ppos);
 }
 
 static int proc_ipc_dointvec_minmax_orphans(struct ctl_table *table, int write,
@@ -99,6 +100,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int zero;
 static int one = 1;
 static int int_max = INT_MAX;
+static int ipc_mni = IPCMNI;
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -120,7 +122,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 		.data		= &init_ipc_ns.shm_ctlmni,
 		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &ipc_mni,
 	},
 	{
 		.procname	= "shm_rmid_forced",
@@ -147,7 +151,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
 		.extra1		= &zero,
-		.extra2		= &int_max,
+		.extra2		= &ipc_mni,
 	},
 	{
 		.procname	= "auto_msgmni",
-- 
1.8.3.1

Re: [PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Andrew Morton]

On Mon, 19 Feb 2018 11:53:50 -0500 Waiman Long <longman@redhat.com> wrote:

> Even with clamped sysctl parameters, it is still not that straight
> forward to figure out the exact range of those parameters. One may
> try to write extreme parameter values to see if they get clamped.
> To make it easier, a warning with the expected range will now be
> printed in the kernel ring buffer when a clamped sysctl parameter
> receives an out of range value.

This assumes that do_proc_dointvec_minmax_conv() and
do_proc_douintvec_minmax_conv() are only ever called by privileged
userspace.  Because we mustn't give unprivileged applications a way to
spam the kernel logs.

That's presumably true in the case of the caller you just added, but I
don't see what we can do to guarantee this in the future, so perhaps we
should add some permission check to the pr_warn()?

Re: [PATCH 1/3] sysctl: Add range clamping intvec helper functions

[Luis R. Rodriguez]

On Mon, Feb 19, 2018 at 11:53:49AM -0500, Waiman Long wrote:
> The current intvec range helper functions will fail with error when
> users try to assign an out-of-range value.

As designed.

> The following new non-failing range helper functions are now added:
>  - proc_dointvec_clamp_minmax()
>  - proc_douintvec_clamp_minmax()
> 
> The new helper functions will clamp the value to within the given
> min/max range without failing it.

The commit log could be a bit more descriptive about that this is a
supplemental additional functionality which is desired. That is, not that its a
bug on the other callers.

One has to read the code to get that the idea here of added functionality
is to be more permissive with lower and upper values which the user inputs,
that when the user input is below the min allowed we clamp down to the
minimum allowed and likewise that when a user input is above the max allowed
we camp to the max allowed.

Adding something like this to the commit log and the documentation for the
new functions should make it very clear to users, otherwise they have to go
read the code.

> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  include/linux/sysctl.h |  6 ++++
>  kernel/sysctl.c        | 97 ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 96 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index b769ecf..7684ea5 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -63,6 +63,12 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
>  				      void __user *, size_t *, loff_t *);
>  extern int proc_do_large_bitmap(struct ctl_table *, int,
>  				void __user *, size_t *, loff_t *);
> +extern int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
> +				      void __user *buffer, size_t *lenp,
> +				      loff_t *ppos);
> +extern int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
> +				       void __user *buffer, size_t *lenp,
> +				       loff_t *ppos);
>  
>  /*
>   * Register a set of sysctl names by calling register_sysctl_table
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f98f28c..f86c3a7 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2500,9 +2500,15 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>  }
>  #endif
>  
> +/*
> + * The clamping flag, if set, will clamp the input value to the range
> + * specified by the given min/max values instead of returning error when
> + * out of range.
> + */
>  struct do_proc_dointvec_minmax_conv_param {
>  	int *min;
>  	int *max;
> +	bool clamp;
>  };

Thanks since you are driving by, do you mind first kdoc'ifying this struct, and then
add your @clamp as a secondary patch with its documentation.

>  
>  static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> @@ -2512,9 +2518,18 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>  	struct do_proc_dointvec_minmax_conv_param *param = data;
>  	if (write) {
>  		int val = *negp ? -*lvalp : *lvalp;
> -		if ((param->min && *param->min > val) ||
> -		    (param->max && *param->max < val))
> -			return -EINVAL;
> +		if (param->min && *param->min > val) {
> +			if (param->clamp)
> +				val = *param->min;
> +			else
> +				return -EINVAL;
> +		}
> +		if (param->max && *param->max < val) {
> +			if (param->clamp)
> +				val = *param->max;
> +			else
> +				return -EINVAL;
> +		}
>  		*valp = val;
>  	} else {
>  		int val = *valp;
> @@ -2556,9 +2571,38 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
>  				do_proc_dointvec_minmax_conv, &param);
>  }
>  
> +/**
> + * proc_dointvec_clamp_minmax - read a vector of integers with min/max values
> + * @table: the sysctl table
> + * @write: %TRUE if this is a write to the sysctl file
> + * @buffer: the user buffer
> + * @lenp: the size of the user buffer
> + * @ppos: file position
> + *
> + * Reads/writes up to table->maxlen/sizeof(unsigned int) integer
> + * values from/to the user buffer, treated as an ASCII string.
> + *
> + * This routine will clamp the values to within the range specified by
> + * table->extra1 (min) and table->extra2 (max).
> + *
> + * Returns 0 on success.
> + */
> +int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
> +		  void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct do_proc_dointvec_minmax_conv_param param = {
> +		.min = (int *) table->extra1,
> +		.max = (int *) table->extra2,
> +		.clamp = true,
> +	};
> +	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> +				do_proc_dointvec_minmax_conv, &param);
> +}
> +
>  struct do_proc_douintvec_minmax_conv_param {
>  	unsigned int *min;
>  	unsigned int *max;
> +	bool clamp;
>  };
>  
>  static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> @@ -2573,10 +2617,18 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>  		if (*lvalp > UINT_MAX)
>  			return -EINVAL;
>  
> -		if ((param->min && *param->min > val) ||
> -		    (param->max && *param->max < val))
> -			return -ERANGE;
> -
> +		if (param->min && *param->min > val) {
> +			if (param->clamp)
> +				val = *param->min;
> +			else
> +				return -ERANGE;
> +		}
> +		if (param->max && *param->max < val) {
> +			if (param->clamp)
> +				val = *param->max;
> +			else
> +				return -ERANGE;
> +		}
>  		*valp = val;
>  	} else {
>  		unsigned int val = *valp;
> @@ -2616,6 +2668,37 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
>  				 do_proc_douintvec_minmax_conv, &param);
>  }
>  
> +/**
> + * proc_douintvec_clamp_minmax - read a vector of uints with min/max values
> + * @table: the sysctl table
> + * @write: %TRUE if this is a write to the sysctl file
> + * @buffer: the user buffer
> + * @lenp: the size of the user buffer
> + * @ppos: file position
> + *
> + * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
> + * values from/to the user buffer, treated as an ASCII string. Negative
> + * strings are not allowed.
> + *
> + * This routine will clamp the values to within the range specified by
> + * table->extra1 (min) and table->extra2 (max). There is a final sanity
> + * check for UINT_MAX to avoid having to support wrap around uses from
> + * userspace.
> + *
> + * Returns 0 on success.
> + */
> +int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
> +			  void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct do_proc_douintvec_minmax_conv_param param = {
> +		.min = (unsigned int *) table->extra1,
> +		.max = (unsigned int *) table->extra2,
> +		.clamp = true,
> +	};
> +	return do_proc_douintvec(table, write, buffer, lenp, ppos,
> +				 do_proc_douintvec_minmax_conv, &param);
> +}
> +
>  static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>  					unsigned int *valp,
>  					int write, void *data)

I realize your user is either built-in or not, as such a modular use is
not yet in order, but can you still just go ahead and add the respective
EXPORT_SYMBOL_GPL() callers as this very likely will get more users soon.

  Luis

Re: [PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Luis R. Rodriguez]

On Tue, Feb 20, 2018 at 03:17:05PM -0800, Andrew Morton wrote:
> On Mon, 19 Feb 2018 11:53:50 -0500 Waiman Long <longman@redhat.com> wrote:
> 
> > Even with clamped sysctl parameters, it is still not that straight
> > forward to figure out the exact range of those parameters. One may
> > try to write extreme parameter values to see if they get clamped.
> > To make it easier, a warning with the expected range will now be
> > printed in the kernel ring buffer when a clamped sysctl parameter
> > receives an out of range value.
> 
> This assumes that do_proc_dointvec_minmax_conv() and
> do_proc_douintvec_minmax_conv() are only ever called by privileged
> userspace.  Because we mustn't give unprivileged applications a way to
> spam the kernel logs.
> 
> That's presumably true in the case of the caller you just added, but I
> don't see what we can do to guarantee this in the future, so perhaps we
> should add some permission check to the pr_warn()?

Beyond what we do with sysctl_perm() on proc_sys_call_handler()?

  Luis

Re: [PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Kees Cook]

On Tue, Feb 20, 2018 at 3:17 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 19 Feb 2018 11:53:50 -0500 Waiman Long <longman@redhat.com> wrote:
>
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
>
> This assumes that do_proc_dointvec_minmax_conv() and
> do_proc_douintvec_minmax_conv() are only ever called by privileged
> userspace.  Because we mustn't give unprivileged applications a way to
> spam the kernel logs.
>
> That's presumably true in the case of the caller you just added, but I
> don't see what we can do to guarantee this in the future, so perhaps we
> should add some permission check to the pr_warn()?

How about pr_warn_ratelimited() instead?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 02/20/2018 06:17 PM, Andrew Morton wrote:
> On Mon, 19 Feb 2018 11:53:50 -0500 Waiman Long <longman@redhat.com> wrote:
>
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
> This assumes that do_proc_dointvec_minmax_conv() and
> do_proc_douintvec_minmax_conv() are only ever called by privileged
> userspace.  Because we mustn't give unprivileged applications a way to
> spam the kernel logs.
>
> That's presumably true in the case of the caller you just added, but I
> don't see what we can do to guarantee this in the future, so perhaps we
> should add some permission check to the pr_warn()?
>
Good point. Beside adding security check, another alternative is to use
some kind of warn_once() for each sysctl parameter. That will limit the
amount of spamming that is possible. It will require adding a flag to
the ctl_table to mark an  entry as warned. I think that will be less
tricky than adding permission check.

I can also use the new flag to designate that a sysctl parameter need to
be clamped to the range instead of failing when out of range. With that,
I don't need to introduce new clamping APIs. I can use the existing
ones. I will work on v2 patch with that change.

Thanks,
Longman

Re: [PATCH 2/3] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 02/20/2018 08:26 PM, Kees Cook wrote:
> On Tue, Feb 20, 2018 at 3:17 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon, 19 Feb 2018 11:53:50 -0500 Waiman Long <longman@redhat.com> wrote:
>>
>>> Even with clamped sysctl parameters, it is still not that straight
>>> forward to figure out the exact range of those parameters. One may
>>> try to write extreme parameter values to see if they get clamped.
>>> To make it easier, a warning with the expected range will now be
>>> printed in the kernel ring buffer when a clamped sysctl parameter
>>> receives an out of range value.
>> This assumes that do_proc_dointvec_minmax_conv() and
>> do_proc_douintvec_minmax_conv() are only ever called by privileged
>> userspace.  Because we mustn't give unprivileged applications a way to
>> spam the kernel logs.
>>
>> That's presumably true in the case of the caller you just added, but I
>> don't see what we can do to guarantee this in the future, so perhaps we
>> should add some permission check to the pr_warn()?
> How about pr_warn_ratelimited() instead?
>
> -Kees
>
My current thinking is to issue at most one warning per sysctl parameter
as additional warning of the same kind does not provide additional
information.

-Longman

Re: [PATCH 1/3] sysctl: Add range clamping intvec helper functions

[Waiman Long]

On 02/20/2018 06:41 PM, Luis R. Rodriguez wrote:
> On Mon, Feb 19, 2018 at 11:53:49AM -0500, Waiman Long wrote:
>> The current intvec range helper functions will fail with error when
>> users try to assign an out-of-range value.
> As designed.
>
>> The following new non-failing range helper functions are now added:
>>  - proc_dointvec_clamp_minmax()
>>  - proc_douintvec_clamp_minmax()
>>
>> The new helper functions will clamp the value to within the given
>> min/max range without failing it.
> The commit log could be a bit more descriptive about that this is a
> supplemental additional functionality which is desired. That is, not that its a
> bug on the other callers.
>
> One has to read the code to get that the idea here of added functionality
> is to be more permissive with lower and upper values which the user inputs,
> that when the user input is below the min allowed we clamp down to the
> minimum allowed and likewise that when a user input is above the max allowed
> we camp to the max allowed.
>
> Adding something like this to the commit log and the documentation for the
> new functions should make it very clear to users, otherwise they have to go
> read the code.

I will provide more information about the need of additional API in the
patch log of the v2 patch.

>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  include/linux/sysctl.h |  6 ++++
>>  kernel/sysctl.c        | 97 ++++++++++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 96 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index b769ecf..7684ea5 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -63,6 +63,12 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
>>  				      void __user *, size_t *, loff_t *);
>>  extern int proc_do_large_bitmap(struct ctl_table *, int,
>>  				void __user *, size_t *, loff_t *);
>> +extern int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
>> +				      void __user *buffer, size_t *lenp,
>> +				      loff_t *ppos);
>> +extern int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
>> +				       void __user *buffer, size_t *lenp,
>> +				       loff_t *ppos);
>>  
>>  /*
>>   * Register a set of sysctl names by calling register_sysctl_table
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index f98f28c..f86c3a7 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2500,9 +2500,15 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>>  }
>>  #endif
>>  
>> +/*
>> + * The clamping flag, if set, will clamp the input value to the range
>> + * specified by the given min/max values instead of returning error when
>> + * out of range.
>> + */
>>  struct do_proc_dointvec_minmax_conv_param {
>>  	int *min;
>>  	int *max;
>> +	bool clamp;
>>  };
> Thanks since you are driving by, do you mind first kdoc'ifying this struct, and then
> add your @clamp as a secondary patch with its documentation.

Sure. Could you provide pointers to some samples kdoc'ified structures?
I know what to do with functions, but not with structures.

>>  
>>  static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>> @@ -2512,9 +2518,18 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>>  	struct do_proc_dointvec_minmax_conv_param *param = data;
>>  	if (write) {
>>  		int val = *negp ? -*lvalp : *lvalp;
>> -		if ((param->min && *param->min > val) ||
>> -		    (param->max && *param->max < val))
>> -			return -EINVAL;
>> +		if (param->min && *param->min > val) {
>> +			if (param->clamp)
>> +				val = *param->min;
>> +			else
>> +				return -EINVAL;
>> +		}
>> +		if (param->max && *param->max < val) {
>> +			if (param->clamp)
>> +				val = *param->max;
>> +			else
>> +				return -EINVAL;
>> +		}
>>  		*valp = val;
>>  	} else {
>>  		int val = *valp;
>> @@ -2556,9 +2571,38 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
>>  				do_proc_dointvec_minmax_conv, &param);
>>  }
>>  
>> +/**
>> + * proc_dointvec_clamp_minmax - read a vector of integers with min/max values
>> + * @table: the sysctl table
>> + * @write: %TRUE if this is a write to the sysctl file
>> + * @buffer: the user buffer
>> + * @lenp: the size of the user buffer
>> + * @ppos: file position
>> + *
>> + * Reads/writes up to table->maxlen/sizeof(unsigned int) integer
>> + * values from/to the user buffer, treated as an ASCII string.
>> + *
>> + * This routine will clamp the values to within the range specified by
>> + * table->extra1 (min) and table->extra2 (max).
>> + *
>> + * Returns 0 on success.
>> + */
>> +int proc_dointvec_clamp_minmax(struct ctl_table *table, int write,
>> +		  void __user *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> +	struct do_proc_dointvec_minmax_conv_param param = {
>> +		.min = (int *) table->extra1,
>> +		.max = (int *) table->extra2,
>> +		.clamp = true,
>> +	};
>> +	return do_proc_dointvec(table, write, buffer, lenp, ppos,
>> +				do_proc_dointvec_minmax_conv, &param);
>> +}
>> +
>>  struct do_proc_douintvec_minmax_conv_param {
>>  	unsigned int *min;
>>  	unsigned int *max;
>> +	bool clamp;
>>  };
>>  
>>  static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>> @@ -2573,10 +2617,18 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>>  		if (*lvalp > UINT_MAX)
>>  			return -EINVAL;
>>  
>> -		if ((param->min && *param->min > val) ||
>> -		    (param->max && *param->max < val))
>> -			return -ERANGE;
>> -
>> +		if (param->min && *param->min > val) {
>> +			if (param->clamp)
>> +				val = *param->min;
>> +			else
>> +				return -ERANGE;
>> +		}
>> +		if (param->max && *param->max < val) {
>> +			if (param->clamp)
>> +				val = *param->max;
>> +			else
>> +				return -ERANGE;
>> +		}
>>  		*valp = val;
>>  	} else {
>>  		unsigned int val = *valp;
>> @@ -2616,6 +2668,37 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
>>  				 do_proc_douintvec_minmax_conv, &param);
>>  }
>>  
>> +/**
>> + * proc_douintvec_clamp_minmax - read a vector of uints with min/max values
>> + * @table: the sysctl table
>> + * @write: %TRUE if this is a write to the sysctl file
>> + * @buffer: the user buffer
>> + * @lenp: the size of the user buffer
>> + * @ppos: file position
>> + *
>> + * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
>> + * values from/to the user buffer, treated as an ASCII string. Negative
>> + * strings are not allowed.
>> + *
>> + * This routine will clamp the values to within the range specified by
>> + * table->extra1 (min) and table->extra2 (max). There is a final sanity
>> + * check for UINT_MAX to avoid having to support wrap around uses from
>> + * userspace.
>> + *
>> + * Returns 0 on success.
>> + */
>> +int proc_douintvec_clamp_minmax(struct ctl_table *table, int write,
>> +			  void __user *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> +	struct do_proc_douintvec_minmax_conv_param param = {
>> +		.min = (unsigned int *) table->extra1,
>> +		.max = (unsigned int *) table->extra2,
>> +		.clamp = true,
>> +	};
>> +	return do_proc_douintvec(table, write, buffer, lenp, ppos,
>> +				 do_proc_douintvec_minmax_conv, &param);
>> +}
>> +
>>  static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>>  					unsigned int *valp,
>>  					int write, void *data)
> I realize your user is either built-in or not, as such a modular use is
> not yet in order, but can you still just go ahead and add the respective
> EXPORT_SYMBOL_GPL() callers as this very likely will get more users soon.

This patch is based on feedback from a customer that it is hard to know
the real range limit of what a sysctl parameter will be. This patch is
the first step in providing this kind of information by the warning in
the ring buffer as well as by reading back the sysctl parameters for
selected parameters. So far, I haven't received any request of using it
in a kernel module.

We can certainly add a patch later on to export the functions when there
is a need to do so.

-Longman