[PATCH 0/4] sysctl: Disallow partial updates of miss-formatted sysctl vectors

[PATCH 0/4] sysctl: Disallow partial updates of miss-formatted sysctl vectors

[Joel Granados]

What?
=====

Stage table->data when writing INT & ULONG ctl_tables. Commit staged
data to kernel variable only when all the conversions have succeeded.
This is applicable only to variables that represent a vector; paths
pertaining to scalers are left unchanged. Notice that partial updates
can still happen if less than the size of the vector are passed and
correctly formatted.

This is the behavior we are protecting against:

  # echo "4 4 1 7" > /proc/sys/kernel/printk
  # echo "1 x" > /proc/sys/kernel/printk
  -bash: echo: write error: Invalid argument
  # cat /proc/sys/kernel/printk
  1 4 1 7    <- the write failed, the first element changed anyway

Why?
====
Allowing a partial change to a vector after returning from an erroneous
proc_handler is just plain wrong. This should be handled within sysctl
to avoid users having to do it for themselves [1]. 

Behavioral Changes
==================
1. A failed write will not update the vector
2. Vector writes can now fail with -ENOMEM

Testing
=======
This went through regular sysctl kunit and self test. Also is posted to
0-day.

I always find it difficult to know who wants to receive this. Please let
me know if you want to be removed from the Cc/To.

Best

[1] https://lore.kernel.org/all/tencent_A860C873956A52E26AD8D309A308A241BA08@qq.com/

Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
Joel Granados (4):
      sysctl: Split data conversion and file position handling
      sysctl: Reject uint arrays before calling the general proc_vec
      sysctl: Disallow partial updates for erroneous sysctl vectors
      sysctl: Add 0013 to test partially updated vectors

 kernel/sysctl.c                          | 228 ++++++++++++++++++++-----------
 tools/testing/selftests/sysctl/sysctl.sh |  57 ++++++++
 2 files changed, 205 insertions(+), 80 deletions(-)
---
base-commit: 4c12287001da60f3c022bb25932aa7a5590fc0b1
change-id: 20260813-lklm-partial_ctlvec-bd8867b70d5c

Best regards,
-- 
Joel Granados <joel.granados@kernel.org>

[PATCH 1/4] sysctl: Split data conversion and file position handling

[Joel Granados]

Apply the conversions to the data in a new helper function
(apply_conv_on_vec) while file position handling and argument validation
stay in the original function. Rename function to prov_vec (from
do_proc_vec). This is a prep commit to isolate the logic that needs to
change to prevent partial sysctl vector writes. No functional change
intended

Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
 kernel/sysctl.c | 202 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 117 insertions(+), 85 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f7b75985d5420a522a316d1fade40dc7fbe60455..ed0e5101949c2fa56e33d543c65175d0ab579fc7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -595,7 +595,7 @@ static int do_proc_int_conv_minmax(bool *negp, unsigned long *u_ptr, int *k_ptr,
 static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
 
 /*
- * Element type processed by do_proc_vec(). The tag selects the element size
+ * Element type processed by proc_vec(). The tag selects the element size
  * and signedness, and it selects which member of union proc_vec_conv is live.
  */
 enum proc_vec_type {
@@ -605,7 +605,7 @@ enum proc_vec_type {
 };
 
 /*
- * Converter passed to do_proc_vec(). Only the member matching the
+ * Converter passed to proc_vec(). Only the member matching the
  * enum proc_vec_type tag is ever read, so every dispatch stays fully typed and
  * no void * converter pointer is needed.
  */
@@ -637,101 +637,133 @@ static int proc_vec_conv(enum proc_vec_type type, union proc_vec_conv conv,
 	return -EINVAL;
 }
 
-/*
- * Read/write a vector of @type elements. The element size and signedness are
- * derived from @type, so a single runtime function replaces the per-type
- * variants. table->data is walked as raw bytes (@i) advanced by @size; the
- * converter performs the actual typed load/store.
+/**
+ * apply_conv_on_vec - Apply converter function on data vector
+ *
+ * Element signedness is derived from @data_type. table->data is walked
+ * as raw bytes (@data) advanced by @data_size; the converter performs
+ * the actual typed load/store. Sets buf_left_final to the number of
+ * bytes that where left outstanding after conversion. Can be > 0.
+ *
+ * Returns 0 on success. Non-zero on error.
  */
-static int do_proc_vec(const struct ctl_table *table, int dir,
-		       void *buffer, size_t *lenp, loff_t *ppos,
-		       enum proc_vec_type type, union proc_vec_conv conv)
+static int apply_conv_on_vec(const union proc_vec_conv conv,
+			     const struct ctl_table *table,
+			     const enum proc_vec_type data_type,
+			     const size_t data_size, const int conv_dir,
+			     const size_t buf_nbyte, void *buf,
+			     size_t *buf_left_final)
 {
-	int vleft, first = 1, err = 0;
-	size_t left, size;
-	bool is_unsigned;
-	char *i, *p;
+	int vec_left, first = 1, err = 0;
+	size_t buf_left;
+	char *data, *p;
+	bool is_unsigned = data_type == PROC_VEC_UINT || data_type == PROC_VEC_ULONG;
 
-	switch (type) {
-	case PROC_VEC_INT:
-		size = sizeof(int);
-		is_unsigned = false;
-		break;
-	case PROC_VEC_UINT:
-		size = sizeof(uint);
-		is_unsigned = true;
-		break;
-	case PROC_VEC_ULONG:
-		size = sizeof(ulong);
-		is_unsigned = true;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	if (!table->data || !table->maxlen || !*lenp ||
-	    (*ppos && SYSCTL_KERN_TO_USER(dir))) {
-		*lenp = 0;
-		return 0;
-	}
-
-	i = table->data;
-	vleft = table->maxlen / size;
-	left = *lenp;
-
-	/* uint arrays are not supported, *Do not* add support for them. */
-	if (type == PROC_VEC_UINT && vleft != 1)
-		return -EINVAL;
-
-	if (SYSCTL_USER_TO_KERN(dir)) {
-		if (proc_first_pos_non_zero_ignore(ppos, table))
-			goto out;
+	data = table->data;
+	vec_left = table->maxlen / data_size;
+	buf_left = buf_nbyte;
 
-		if (left > PAGE_SIZE - 1)
-			left = PAGE_SIZE - 1;
-		p = buffer;
+	if (SYSCTL_USER_TO_KERN(conv_dir)) {
+		if (buf_left > PAGE_SIZE - 1)
+			buf_left = PAGE_SIZE - 1;
+		p = buf;
 	}
 
-	for (; left && vleft--; i += size, first = 0) {
+	for (; buf_left && vec_left--; data += data_size, first = 0) {
 		unsigned long lval;
 		bool neg = false;
 
-		if (SYSCTL_USER_TO_KERN(dir)) {
-			proc_skip_spaces(&p, &left);
+		if (SYSCTL_USER_TO_KERN(conv_dir)) {
+			proc_skip_spaces(&p, &buf_left);
 
-			if (!left)
+			if (!buf_left)
 				break;
-			err = proc_get_long(&p, &left, &lval, &neg,
+			err = proc_get_long(&p, &buf_left, &lval, &neg,
 					    proc_wspace_sep,
 					    sizeof(proc_wspace_sep), NULL);
 			if (!err && neg && is_unsigned)
 				err = -EINVAL;
 			if (err)
 				break;
-			if (proc_vec_conv(type, conv, &neg, &lval, i, dir, table)) {
+			if (proc_vec_conv(data_type, conv, &neg, &lval, data, conv_dir, table)) {
 				err = -EINVAL;
 				break;
 			}
 		} else {
-			if (proc_vec_conv(type, conv, &neg, &lval, i, dir, table)) {
+			if (proc_vec_conv(data_type, conv, &neg, &lval, data, conv_dir, table)) {
 				err = -EINVAL;
 				break;
 			}
 			if (!first)
-				proc_put_char(&buffer, &left, '\t');
-			proc_put_long(&buffer, &left, lval, neg);
+				proc_put_char(&buf, &buf_left, '\t');
+			proc_put_long(&buf, &buf_left, lval, neg);
 		}
 	}
 
-	if (SYSCTL_KERN_TO_USER(dir) && !first && left && !err)
-		proc_put_char(&buffer, &left, '\n');
-	if (SYSCTL_USER_TO_KERN(dir) && !err && left)
-		proc_skip_spaces(&p, &left);
-	if (SYSCTL_USER_TO_KERN(dir) && first)
+	if (SYSCTL_KERN_TO_USER(conv_dir) && !first && buf_left && !err)
+		proc_put_char(&buf, &buf_left, '\n');
+	if (SYSCTL_USER_TO_KERN(conv_dir) && !err && buf_left)
+		proc_skip_spaces(&p, &buf_left);
+	if (SYSCTL_USER_TO_KERN(conv_dir) && first)
 		return err ? : -EINVAL;
-	*lenp -= left;
+	*buf_left_final = buf_left;
+
+	return err;
+}
+
+/* Read/write a vector of @type elements. */
+static int proc_vec(const struct ctl_table *table, int dir, void *buffer,
+		    size_t *lenp, loff_t *ppos, enum proc_vec_type type,
+		    union proc_vec_conv conv)
+{
+	int err = 0;
+	size_t data_size, left_nbyte = SIZE_MAX;
+
+	switch (type) {
+	case PROC_VEC_INT:
+		data_size = sizeof(int);
+		break;
+	case PROC_VEC_UINT:
+		data_size = sizeof(uint);
+		break;
+	case PROC_VEC_ULONG:
+		data_size = sizeof(ulong);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!table->data || !table->maxlen || !*lenp ||
+	    (*ppos && SYSCTL_KERN_TO_USER(dir))) {
+		*lenp = 0;
+		return 0;
+	}
+
+	/* uint arrays are not supported, *Do not* add support for them. */
+	if (type == PROC_VEC_UINT && (table->maxlen / data_size) != 1)
+		return -EINVAL;
+
+	if (SYSCTL_USER_TO_KERN(dir)) {
+		if (proc_first_pos_non_zero_ignore(ppos, table))
+			goto out;
+	}
+
+	err = apply_conv_on_vec(conv, table, type, data_size, dir, *lenp, buffer,
+				&left_nbyte);
+
+	/*
+	 * An unchanged left_nbyte signals a write with no parsed element; which
+	 * is an error. Using SIZE_MAX to detect this error is possible because:
+	 * 1. lenp is bounded by KMALLOC_MAX_SIZE in proc_sys_call_handler
+	 * 2. lenp could never be SIZE_MAX as it is a "ridiculous" (exabyte) allocation.
+	 */
+	if (left_nbyte == SIZE_MAX)
+		return err;
+
+	*lenp -= left_nbyte;
 out:
 	*ppos += *lenp;
+
 	return err;
 }
 
@@ -760,8 +792,8 @@ int proc_douintvec_conv(const struct ctl_table *table, int dir, void *buffer,
 	if (!conv)
 		conv = do_proc_uint_conv;
 
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			   (union proc_vec_conv){ .uint_conv = conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
+			(union proc_vec_conv){ .uint_conv = conv });
 }
 
 /**
@@ -820,8 +852,8 @@ int proc_dobool(const struct ctl_table *table, int dir, void *buffer,
 int proc_dointvec(const struct ctl_table *table, int dir, void *buffer,
 		  size_t *lenp, loff_t *ppos)
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
-			   (union proc_vec_conv){ .int_conv = do_proc_int_conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
+			(union proc_vec_conv){ .int_conv = do_proc_int_conv });
 }
 
 /**
@@ -840,8 +872,8 @@ int proc_dointvec(const struct ctl_table *table, int dir, void *buffer,
 int proc_douintvec(const struct ctl_table *table, int dir, void *buffer,
 		size_t *lenp, loff_t *ppos)
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			   (union proc_vec_conv){ .uint_conv = do_proc_uint_conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
+			(union proc_vec_conv){ .uint_conv = do_proc_uint_conv });
 }
 
 /**
@@ -864,8 +896,8 @@ int proc_douintvec(const struct ctl_table *table, int dir, void *buffer,
 int proc_dointvec_minmax(const struct ctl_table *table, int dir,
 		  void *buffer, size_t *lenp, loff_t *ppos)
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
-			   (union proc_vec_conv){ .int_conv = do_proc_int_conv_minmax });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
+			(union proc_vec_conv){ .int_conv = do_proc_int_conv_minmax });
 }
 
 /**
@@ -891,8 +923,8 @@ int proc_dointvec_minmax(const struct ctl_table *table, int dir,
 int proc_douintvec_minmax(const struct ctl_table *table, int dir,
 			  void *buffer, size_t *lenp, loff_t *ppos)
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			   (union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
+			(union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
 }
 
 /**
@@ -935,8 +967,8 @@ int proc_dou8vec_minmax(const struct ctl_table *table, int dir,
 		tmp.extra2 = (unsigned int *) &max;
 
 	val = READ_ONCE(*data);
-	res = do_proc_vec(&tmp, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			  (union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
+	res = proc_vec(&tmp, dir, buffer, lenp, ppos, PROC_VEC_UINT,
+		       (union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
 	if (res)
 		return res;
 	if (SYSCTL_USER_TO_KERN(dir))
@@ -1066,8 +1098,8 @@ int proc_doulongvec_conv(const struct ctl_table *table, int dir,
 				int (*conv)(bool *negp, ulong *u_ptr, ulong *k_ptr,
 					    int dir, const struct ctl_table *table))
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_ULONG,
-			   (union proc_vec_conv){ .ulong_conv = conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_ULONG,
+			(union proc_vec_conv){ .ulong_conv = conv });
 }
 
 /**
@@ -1087,10 +1119,10 @@ int proc_doulongvec_conv(const struct ctl_table *table, int dir,
  * Returns: %0 on success.
  */
 int proc_doulongvec_minmax(const struct ctl_table *table, int dir,
-			   void *buffer, size_t *lenp, loff_t *ppos)
+		void *buffer, size_t *lenp, loff_t *ppos)
 {
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_ULONG,
-			   (union proc_vec_conv){ .ulong_conv = do_proc_ulong_conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_ULONG,
+			(union proc_vec_conv){ .ulong_conv = do_proc_ulong_conv });
 }
 
 /**
@@ -1114,8 +1146,8 @@ int proc_dointvec_conv(const struct ctl_table *table, int dir, void *buffer,
 {
 	if (!conv)
 		conv = do_proc_int_conv;
-	return do_proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
-			   (union proc_vec_conv){ .int_conv = conv });
+	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_INT,
+			(union proc_vec_conv){ .int_conv = conv });
 }
 
 /**

-- 
2.50.1

[PATCH 2/4] sysctl: Reject uint arrays before calling the general proc_vec

[Joel Granados]

Move the UINT vector size check to proc_douintvec_conv; the function
that routes UINT types only. Route all the UINT calls (including
proc_dou8vec_minmax) through proc_douintvec_conv.

UINT proc handlers that incorrectly define maxlen will now return
-EINVAL instead of 0 in the cases where data is missing, lenp is 0 or
ppos is 0. Note that maxlen == 0 is not considered as miss-defined.

Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
 kernel/sysctl.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ed0e5101949c2fa56e33d543c65175d0ab579fc7..c5fa916e626a336c004d596f4c74f829b1cdc5e1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -739,10 +739,6 @@ static int proc_vec(const struct ctl_table *table, int dir, void *buffer,
 		return 0;
 	}
 
-	/* uint arrays are not supported, *Do not* add support for them. */
-	if (type == PROC_VEC_UINT && (table->maxlen / data_size) != 1)
-		return -EINVAL;
-
 	if (SYSCTL_USER_TO_KERN(dir)) {
 		if (proc_first_pos_non_zero_ignore(ppos, table))
 			goto out;
@@ -788,6 +784,9 @@ int proc_douintvec_conv(const struct ctl_table *table, int dir, void *buffer,
 			int (*conv)(bool *negp, ulong *u_ptr, uint *k_ptr,
 				    int dir, const struct ctl_table *table))
 {
+	/* uint arrays are not supported, *Do not* add support for them. */
+	if (table->maxlen && (table->maxlen / sizeof(uint)) != 1)
+		return -EINVAL;
 
 	if (!conv)
 		conv = do_proc_uint_conv;
@@ -872,8 +871,7 @@ int proc_dointvec(const struct ctl_table *table, int dir, void *buffer,
 int proc_douintvec(const struct ctl_table *table, int dir, void *buffer,
 		size_t *lenp, loff_t *ppos)
 {
-	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			(union proc_vec_conv){ .uint_conv = do_proc_uint_conv });
+	return proc_douintvec_conv(table, dir, buffer, lenp, ppos, do_proc_uint_conv);
 }
 
 /**
@@ -923,8 +921,8 @@ int proc_dointvec_minmax(const struct ctl_table *table, int dir,
 int proc_douintvec_minmax(const struct ctl_table *table, int dir,
 			  void *buffer, size_t *lenp, loff_t *ppos)
 {
-	return proc_vec(table, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-			(union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
+	return proc_douintvec_conv(table, dir, buffer, lenp, ppos,
+				   do_proc_uint_conv_minmax);
 }
 
 /**
@@ -967,8 +965,7 @@ int proc_dou8vec_minmax(const struct ctl_table *table, int dir,
 		tmp.extra2 = (unsigned int *) &max;
 
 	val = READ_ONCE(*data);
-	res = proc_vec(&tmp, dir, buffer, lenp, ppos, PROC_VEC_UINT,
-		       (union proc_vec_conv){ .uint_conv = do_proc_uint_conv_minmax });
+	res = proc_douintvec_minmax(&tmp, dir, buffer, lenp, ppos);
 	if (res)
 		return res;
 	if (SYSCTL_USER_TO_KERN(dir))

-- 
2.50.1

[PATCH 3/4] sysctl: Disallow partial updates for erroneous sysctl vectors

[Joel Granados]

When updating the kernel sysctl vectors there is a chance that not all
vector elements are updated due to erroneous input. Use a staging
variable that holds a copy of the vector and commits to the actual
table->data only when all input is successfully updated.

The staging is only for vectors; cases where table->data points to a
variable should not be staged as they will not be updated on input
error. PROC_VEC_UINT is not included because UINT arrays are not
allowed.

Replace first with nr_conv, incremented where first was cleared. first
is exactly nr_conv == 0, and the counter doubles as the number of
elements to publish.

Example of behavior that is being prevented:

  # echo "4 4 1 7" > /proc/sys/kernel/printk
  # echo "1 x" > /proc/sys/kernel/printk
  -bash: echo: write error: Invalid argument
  # cat /proc/sys/kernel/printk
  1 4 1 7 <- incorrect

It should be unchanged ("4 4 1 7") on error.

Link: https://lore.kernel.org/all/tencent_A860C873956A52E26AD8D309A308A241BA08@qq.com/
Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
 kernel/sysctl.c | 65 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c5fa916e626a336c004d596f4c74f829b1cdc5e1..787f53d70507b1583518f86d50d526828462a902 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -637,6 +637,26 @@ static int proc_vec_conv(enum proc_vec_type type, union proc_vec_conv conv,
 	return -EINVAL;
 }
 
+static int commit_conv_vec(const enum proc_vec_type data_type, void *dst,
+			    const void *src, size_t nr)
+{
+	size_t i;
+
+	switch (data_type) {
+	case PROC_VEC_INT:
+		for (i = 0; i < nr; i++)
+			WRITE_ONCE(((int *)dst)[i], ((const int *)src)[i]);
+		return 0;
+
+	case PROC_VEC_ULONG:
+		for (i = 0; i < nr; i++)
+			WRITE_ONCE(((ulong *)dst)[i], ((const ulong *)src)[i]);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 /**
  * apply_conv_on_vec - Apply converter function on data vector
  *
@@ -654,22 +674,31 @@ static int apply_conv_on_vec(const union proc_vec_conv conv,
 			     const size_t buf_nbyte, void *buf,
 			     size_t *buf_left_final)
 {
-	int vec_left, first = 1, err = 0;
-	size_t buf_left;
-	char *data, *p;
+	int vec_left, err = 0;
+	size_t buf_left, nr_conv = 0;
+	char *data, *data_stage = NULL, *p;
 	bool is_unsigned = data_type == PROC_VEC_UINT || data_type == PROC_VEC_ULONG;
 
-	data = table->data;
-	vec_left = table->maxlen / data_size;
 	buf_left = buf_nbyte;
 
+	data = table->data;
 	if (SYSCTL_USER_TO_KERN(conv_dir)) {
 		if (buf_left > PAGE_SIZE - 1)
 			buf_left = PAGE_SIZE - 1;
 		p = buf;
+
+		if (table->maxlen > data_size) {
+			data_stage = kmemdup(table->data, table->maxlen, GFP_KERNEL);
+			if (!data_stage) {
+				err = -ENOMEM;
+				goto out;
+			}
+			data = data_stage;
+		}
 	}
 
-	for (; buf_left && vec_left--; data += data_size, first = 0) {
+	vec_left = table->maxlen / data_size;
+	for (; buf_left && vec_left--; data += data_size, nr_conv++) {
 		unsigned long lval;
 		bool neg = false;
 
@@ -694,20 +723,30 @@ static int apply_conv_on_vec(const union proc_vec_conv conv,
 				err = -EINVAL;
 				break;
 			}
-			if (!first)
+			if (nr_conv)
 				proc_put_char(&buf, &buf_left, '\t');
 			proc_put_long(&buf, &buf_left, lval, neg);
 		}
 	}
 
-	if (SYSCTL_KERN_TO_USER(conv_dir) && !first && buf_left && !err)
-		proc_put_char(&buf, &buf_left, '\n');
-	if (SYSCTL_USER_TO_KERN(conv_dir) && !err && buf_left)
-		proc_skip_spaces(&p, &buf_left);
-	if (SYSCTL_USER_TO_KERN(conv_dir) && first)
-		return err ? : -EINVAL;
+	if (SYSCTL_USER_TO_KERN(conv_dir)) {
+		if (!err && buf_left)
+			proc_skip_spaces(&p, &buf_left);
+		if (!nr_conv) {
+			err = err ? : -EINVAL;
+			goto out;
+		}
+		if (!err && data_stage)
+			err = commit_conv_vec(data_type, table->data, data_stage, nr_conv);
+	} else {
+		if (nr_conv && buf_left && !err)
+			proc_put_char(&buf, &buf_left, '\n');
+	}
+
 	*buf_left_final = buf_left;
 
+out:
+	kfree(data_stage);
 	return err;
 }
 

-- 
2.50.1

[PATCH 4/4] sysctl: Add 0013 to test partially updated vectors

[Joel Granados]

Add coverage for partially updated sysctl vectors with test 0013. It
makes sure that the sysctl vector is unchanged when one of the elements
is not valid.

Signed-off-by: Joel Granados <joel.granados@kernel.org>
---
 tools/testing/selftests/sysctl/sysctl.sh | 57 ++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index b2d8bd9026a721e96c26be20069f7f87b06a5cf7..fdf3d2fb0a2ad23a525d3205577a06d7d97576b7 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -4,6 +4,7 @@
 
 # This performs a series tests against the proc sysctl interface.
 
+# shellcheck disable=SC2317
 # Kselftest framework requirement - SKIP code is 4.
 ksft_skip=4
 
@@ -37,6 +38,7 @@ ALL_TESTS="$ALL_TESTS 0009:1:1:unregister_error:0"
 ALL_TESTS="$ALL_TESTS 0010:1:1:mnt/mnt_error:0"
 ALL_TESTS="$ALL_TESTS 0011:1:1:empty_add:0"
 ALL_TESTS="$ALL_TESTS 0012:1:1:u8_valid:0"
+ALL_TESTS="$ALL_TESTS 0013:1:1:int_0003:1"
 
 function allow_user_defaults()
 {
@@ -196,6 +198,18 @@ verify_diff_w()
 	return $?
 }
 
+# Verify that an erroneous ($2) update fails and does not change TARGET
+verify_no_partial_update()
+{
+	TEST_STR="$1"
+	echo -n "$TEST_STR" > "$TARGET"
+
+	if echo -n "$2" > "$TARGET" 2> /dev/null; then
+		return 1
+	fi
+	verify_diff_w "${TARGET}"
+}
+
 test_rc()
 {
 	if [[ $rc != 0 ]]; then
@@ -501,6 +515,40 @@ run_limit_digit_int_array()
 	test_rc
 }
 
+# You used an int array and one of the elements is not acceptable
+run_int_array_no_partial_update()
+{
+	echo -n "Testing invalid array element does not partially update ... "
+        # Expect failure because of 3rd element (abc).
+	if ! verify_no_partial_update "1 2 3 4" "10 20 abc 40"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "OK"
+	fi
+	test_rc
+
+	echo -n "Testing out of range array element does not partially update ... "
+        # Expect failure because of 3rd element (greater than int)
+	if ! verify_no_partial_update "1 2 3 4" "10 20 $((INT_MAX + 1)) 40"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "OK"
+	fi
+	test_rc
+
+	echo -n "Testing invalid first array element does not update ... "
+        # Expect failure of 1st element
+	if ! verify_no_partial_update "1 2 3 4" "abc 20 30 40"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "OK"
+	fi
+	test_rc
+}
+
 # You are using an unsigned int
 run_limit_digit_uint()
 {
@@ -880,6 +928,14 @@ sysctl_test_0012()
 	return 0
 }
 
+sysctl_test_0013()
+{
+	TARGET="${SYSCTL}/$(get_test_target 0013)"
+	reset_vals
+
+	run_int_array_no_partial_update
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -900,6 +956,7 @@ list_tests()
 	echo "0010 x $(get_test_count 0010) - tests sysct mount point"
 	echo "0011 x $(get_test_count 0011) - tests empty directories"
 	echo "0012 x $(get_test_count 0012) - tests range check for u8 proc_handler"
+	echo "0013 x $(get_test_count 0013) - tests partially update vectors on error"
 }
 
 usage()

-- 
2.50.1