Re: [PATCH v3] sysctl: fix check against uninitialized variable in proc_do_large_bitmap

[Marc Buerg <buermarc@googlemail.com>]

Hi Peter, Hi Kees,

Thanks for your responses.

On Fri, 20 Mar 2026 11:30:29 -0700, Kees Cook <kees@kernel.org> wrote:

> Please don't include "---" as a separator here: we want to keep your
> entire commit log (including the SoB and other tags).

Ah, sorry, I will fix that. I guess the SoB and tags have to go into the actual
commit message and not into the cover letter of b4. My understanding is that
additional bug information can be kept where it is, but the tags must be within
the commit message.

On Sat, 21 Mar 2026 13:05:26 +0100, Peter Seiderer wrote:

> > Also, I think the explicit zero-init of 'c' would be nice to keep just
> > for robustness: the compiler can elide it if it decides it's a duplicate
> > store. There's only upside to including it.
> 
> Sorry, disagree on this one, not a fan of this 'add unneeded code just in =
> case...'
> pattern hiding the real fix and/or logic of the code, but just the
> opinion of a sporadic contributor ;-)
> 
> @Marc: in case you go this way, please remove my Reviewed-by tag on next
> patch iteration...

I think I have to revisit the fix in general. I did read the review from
shasiko [1] and adding only the left check will not return an -EINVAL for a
trailing dash, e.g. '123-' would return 0 and set the bitmap to 123. Previously
this would return -EINVAL. I added a local test to kernel/sysctl-test.c to
verify this.

The problem is that if left is non zero we advance the pointer and reduce left
after the proc_get_long() call. Normally if we had a trailing dash we would
take the second proc_get_long() call and receive an -EINVAL if nothing is
present anymore. So even if we have a trailing character, left will not be a
correct indicator anymore when we want to check c.

We could either not check left and rely on just c = 0, or introduce a new
variable that stores the information that c has been filled. I will have to
think some more about it, but wanted to already share the finding.

I will provide a new patch at some point, but will think a bit more about
anything I have missed. Until then I tried to send this response in form of a
format patch to provide the test and changes for better understanding. The
patch was created with 80234b5ab240f52fa45d201e899e207b9265ef91 as the parent
similar to PATCH v3.

Best Regards,
Marc

[1] https://sashiko.dev/#/patchset/20260319-fix-uninitialized-variable-in-proc_do_large_bitmap-v3-1-9cfc3ff60c09%40googlemail.com
---
 kernel/sysctl-test.c | 27 +++++++++++++++++++++++++++
 kernel/sysctl.c      |  9 +++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
index 92f94ea28957..7fc1ed232cd3 100644
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -367,6 +367,32 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
 	KUNIT_EXPECT_EQ(test, 0, *((int *)table.data));
 }
 
+static void
+sysctl_test_proc_do_large_with_invalid_range(struct kunit *test)
+{
+	DECLARE_BITMAP(bitmap, 1024);
+	bitmap_zero(bitmap, 1024);
+
+	unsigned long *bitmap_ptr = bitmap;
+	struct ctl_table test_data = {
+		.data = &bitmap_ptr,
+		.maxlen = 1024,
+	};
+
+	char input[] = "123-";
+	size_t len = sizeof(input) - 1;
+	loff_t pos = 0;
+
+	char *buffer = kunit_kzalloc(test, sizeof(input), GFP_USER);
+	char __user *user_buffer = (char __user *)buffer;
+	memcpy(buffer, input, sizeof(input));
+
+	int result = proc_do_large_bitmap(&test_data, 1, user_buffer, &len, &pos);
+	KUNIT_EXPECT_EQ(test, result, -22);
+	KUNIT_EXPECT_FALSE(test, test_bit(123, bitmap));
+}
+
+
 static struct kunit_case sysctl_test_cases[] = {
 	KUNIT_CASE(sysctl_test_api_dointvec_null_tbl_data),
 	KUNIT_CASE(sysctl_test_api_dointvec_table_maxlen_unset),
@@ -378,6 +404,7 @@ static struct kunit_case sysctl_test_cases[] = {
 	KUNIT_CASE(sysctl_test_dointvec_write_happy_single_negative),
 	KUNIT_CASE(sysctl_test_api_dointvec_write_single_less_int_min),
 	KUNIT_CASE(sysctl_test_api_dointvec_write_single_greater_int_max),
+	KUNIT_CASE(sysctl_test_proc_do_large_with_invalid_range),
 	{}
 };
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 9d3a666ffde1..041fbc3e73e7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1118,7 +1118,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
 	unsigned long bitmap_len = table->maxlen;
 	unsigned long *bitmap = *(unsigned long **) table->data;
 	unsigned long *tmp_bitmap = NULL;
-	char tr_a[] = { '-', ',', '\n' }, tr_b[] = { ',', '\n', 0 }, c;
+	char tr_a[] = { '-', ',', '\n' }, tr_b[] = { ',', '\n', 0 }, c = 0;
 
 	if (!bitmap || !bitmap_len || !left || (*ppos && SYSCTL_KERN_TO_USER(dir))) {
 		*lenp = 0;
@@ -1143,6 +1143,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
 			unsigned long val_a, val_b;
 			bool neg;
 			size_t saved_left;
+			bool trailing_character_set = false;
 
 			/* In case we stop parsing mid-number, we can reset */
 			saved_left = left;
@@ -1158,6 +1159,9 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
 				break;
 			}
 
+			if (left)
+				trailing_character_set = true;
+
 			if (err)
 				break;
 			if (val_a >= bitmap_len || neg) {
@@ -1166,12 +1170,13 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
 			}
 
 			val_b = val_a;
+
 			if (left) {
 				p++;
 				left--;
 			}
 
-			if (c == '-') {
+			if (trailing_character_set && c == '-') {
 				err = proc_get_long(&p, &left, &val_b,
 						     &neg, tr_b, sizeof(tr_b),
 						     &c);
-- 
2.51.0