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

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

[Marc Buerg]

proc_do_large_bitmap() does not initialize variable c, which is expected
to be set to a trailing character by proc_get_long().

However, proc_get_long() only sets c when the input buffer contains a
trailing character after the parsed value.

If c is not initialized it may happen to contain a '-'. If this is the
case proc_do_large_bitmap() expects to be able to parse a second part of
the input buffer. If there is no second part an unjustified -EINVAL will
be returned.

Add check that left is non-zero before checking c, as proc_get_long()
ensures that the passed left is non-zero, if a trailing character
exists.

---
When writing to /proc/sys/net/ipv4/ip_local_reserved_ports it is
possible to receive an -EINVAL for a valid value.

This happens due to a check of a potentially uninitialized variable in
the proc_do_large_bitmap() function, namely char c. To trigger this
behavior the variable has to contain the later explicitly checked '-'
char by chance.

In proc_do_large_bitmap() it is expected that the variable might be
filled by the proc_get_long() function with the trailing character of
the given input. But only if a trailing character exists within the
passed size of the buffer.

If no trailing character is present we still do a c == '-' check. If the
uninitialized variable contains this char the function continues
parsing. It will now set err to -EINVAL in the next proc_get_long()
call, as there is nothing more to parse.

proc_do_large_bitmap() passes left to the proc_get_long() call. left
will only be non-zero, if a trailing character has been written.
Therefore, checking that left is non-zero before accessing c fixes this
problem.

The problem will only arise sporadically, as the variable must contain
'-' by chance. On the affected system CONFIG_INIT_STACK_NONE=y was
enabled. Further, when enabling eBPF tracing to dump contents of the
stack the issue disappeared.

Fixes: 9f977fb7ae9d ("sysctl: add proc_do_large_bitmap")
Signed-off-by: Marc Buerg <buermarc@googlemail.com>
Reviewed-by: Peter Seiderer <ps.report@gmx.net>
---
Changes in v3:
- Add Reviewed-by: Peter Seiderer <ps.report@gmx.net>
- Re-include bug context into cover letter
- Link to v2: https://lore.kernel.org/r/20260317-fix-uninitialized-variable-in-proc_do_large_bitmap-v2-1-6dfb1aefa287@googlemail.com

Changes in v2:
- Drop initialization of c to 0
- Include checking that left is non-zero before checking against c
- Link to v1: https://lore.kernel.org/r/20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-v1-1-35ad2dddaf21@googlemail.com
---
 kernel/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 9d3a666ffde1..dd337a63da41 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1171,7 +1171,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
 				left--;
 			}
 
-			if (c == '-') {
+			if (left && c == '-') {
 				err = proc_get_long(&p, &left, &val_b,
 						     &neg, tr_b, sizeof(tr_b),
 						     &c);

---
base-commit: 80234b5ab240f52fa45d201e899e207b9265ef91
change-id: 20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-30c6ef4ac1c5

Best regards,
-- 
buermarc <buermarc@googlemail.com>

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

[Kees Cook]

On Thu, Mar 19, 2026 at 11:50:50PM +0100, Marc Buerg wrote:
> proc_do_large_bitmap() does not initialize variable c, which is expected
> to be set to a trailing character by proc_get_long().
> 
> However, proc_get_long() only sets c when the input buffer contains a
> trailing character after the parsed value.
> 
> If c is not initialized it may happen to contain a '-'. If this is the
> case proc_do_large_bitmap() expects to be able to parse a second part of
> the input buffer. If there is no second part an unjustified -EINVAL will
> be returned.
> 
> Add check that left is non-zero before checking c, as proc_get_long()
> ensures that the passed left is non-zero, if a trailing character
> exists.
> 
> ---

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

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.

-Kees

> When writing to /proc/sys/net/ipv4/ip_local_reserved_ports it is
> possible to receive an -EINVAL for a valid value.
> 
> This happens due to a check of a potentially uninitialized variable in
> the proc_do_large_bitmap() function, namely char c. To trigger this
> behavior the variable has to contain the later explicitly checked '-'
> char by chance.
> 
> In proc_do_large_bitmap() it is expected that the variable might be
> filled by the proc_get_long() function with the trailing character of
> the given input. But only if a trailing character exists within the
> passed size of the buffer.
> 
> If no trailing character is present we still do a c == '-' check. If the
> uninitialized variable contains this char the function continues
> parsing. It will now set err to -EINVAL in the next proc_get_long()
> call, as there is nothing more to parse.
> 
> proc_do_large_bitmap() passes left to the proc_get_long() call. left
> will only be non-zero, if a trailing character has been written.
> Therefore, checking that left is non-zero before accessing c fixes this
> problem.
> 
> The problem will only arise sporadically, as the variable must contain
> '-' by chance. On the affected system CONFIG_INIT_STACK_NONE=y was
> enabled. Further, when enabling eBPF tracing to dump contents of the
> stack the issue disappeared.
> 
> Fixes: 9f977fb7ae9d ("sysctl: add proc_do_large_bitmap")
> Signed-off-by: Marc Buerg <buermarc@googlemail.com>
> Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> ---
> Changes in v3:
> - Add Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> - Re-include bug context into cover letter
> - Link to v2: https://lore.kernel.org/r/20260317-fix-uninitialized-variable-in-proc_do_large_bitmap-v2-1-6dfb1aefa287@googlemail.com
> 
> Changes in v2:
> - Drop initialization of c to 0
> - Include checking that left is non-zero before checking against c
> - Link to v1: https://lore.kernel.org/r/20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-v1-1-35ad2dddaf21@googlemail.com
> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 9d3a666ffde1..dd337a63da41 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1171,7 +1171,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
>  				left--;
>  			}
>  
> -			if (c == '-') {
> +			if (left && c == '-') {
>  				err = proc_get_long(&p, &left, &val_b,
>  						     &neg, tr_b, sizeof(tr_b),
>  						     &c);
> 
> ---
> base-commit: 80234b5ab240f52fa45d201e899e207b9265ef91
> change-id: 20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-30c6ef4ac1c5
> 
> Best regards,
> -- 
> buermarc <buermarc@googlemail.com>
> 

-- 
Kees Cook

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

[Peter Seiderer]

Hello Marc, Kees,

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

> On Thu, Mar 19, 2026 at 11:50:50PM +0100, Marc Buerg wrote:
> > proc_do_large_bitmap() does not initialize variable c, which is expected
> > to be set to a trailing character by proc_get_long().
> > 
> > However, proc_get_long() only sets c when the input buffer contains a
> > trailing character after the parsed value.
> > 
> > If c is not initialized it may happen to contain a '-'. If this is the
> > case proc_do_large_bitmap() expects to be able to parse a second part of
> > the input buffer. If there is no second part an unjustified -EINVAL will
> > be returned.
> > 
> > Add check that left is non-zero before checking c, as proc_get_long()
> > ensures that the passed left is non-zero, if a trailing character
> > exists.
> > 
> > ---  
> 
> Please don't include "---" as a separator here: we want to keep your
> entire commit log (including the SoB and other tags).

+1 for this one...

> 
> 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...

Regards,
Peter

> 
> -Kees
> 
> > When writing to /proc/sys/net/ipv4/ip_local_reserved_ports it is
> > possible to receive an -EINVAL for a valid value.
> > 
> > This happens due to a check of a potentially uninitialized variable in
> > the proc_do_large_bitmap() function, namely char c. To trigger this
> > behavior the variable has to contain the later explicitly checked '-'
> > char by chance.
> > 
> > In proc_do_large_bitmap() it is expected that the variable might be
> > filled by the proc_get_long() function with the trailing character of
> > the given input. But only if a trailing character exists within the
> > passed size of the buffer.
> > 
> > If no trailing character is present we still do a c == '-' check. If the
> > uninitialized variable contains this char the function continues
> > parsing. It will now set err to -EINVAL in the next proc_get_long()
> > call, as there is nothing more to parse.
> > 
> > proc_do_large_bitmap() passes left to the proc_get_long() call. left
> > will only be non-zero, if a trailing character has been written.
> > Therefore, checking that left is non-zero before accessing c fixes this
> > problem.
> > 
> > The problem will only arise sporadically, as the variable must contain
> > '-' by chance. On the affected system CONFIG_INIT_STACK_NONE=y was
> > enabled. Further, when enabling eBPF tracing to dump contents of the
> > stack the issue disappeared.
> > 
> > Fixes: 9f977fb7ae9d ("sysctl: add proc_do_large_bitmap")
> > Signed-off-by: Marc Buerg <buermarc@googlemail.com>
> > Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> > ---
> > Changes in v3:
> > - Add Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> > - Re-include bug context into cover letter
> > - Link to v2: https://lore.kernel.org/r/20260317-fix-uninitialized-variable-in-proc_do_large_bitmap-v2-1-6dfb1aefa287@googlemail.com
> > 
> > Changes in v2:
> > - Drop initialization of c to 0
> > - Include checking that left is non-zero before checking against c
> > - Link to v1: https://lore.kernel.org/r/20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-v1-1-35ad2dddaf21@googlemail.com
> > ---
> >  kernel/sysctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 9d3a666ffde1..dd337a63da41 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -1171,7 +1171,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
> >  				left--;
> >  			}
> >  
> > -			if (c == '-') {
> > +			if (left && c == '-') {
> >  				err = proc_get_long(&p, &left, &val_b,
> >  						     &neg, tr_b, sizeof(tr_b),
> >  						     &c);
> > 
> > ---
> > base-commit: 80234b5ab240f52fa45d201e899e207b9265ef91
> > change-id: 20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-30c6ef4ac1c5
> > 
> > Best regards,
> > -- 
> > buermarc <buermarc@googlemail.com>
> >   
> 

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

[Marc Buerg]

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

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 4374 bytes --]

On Thu, Mar 19, 2026 at 11:50:50PM +0100, Marc Buerg wrote:
> proc_do_large_bitmap() does not initialize variable c, which is expected
> to be set to a trailing character by proc_get_long().
Ack.

> 
> However, proc_get_long() only sets c when the input buffer contains a
> trailing character after the parsed value.
Ack.

> 
> If c is not initialized it may happen to contain a '-'. If this is the
Here I have a question:

How is it possible that the buffer in proc_do_large_bitmap is not
terminated with a '\0' when writing?

The proc_do_large_bitmap gets called from proc_sys_call_handler and
before running the table->proc_handler call, the end of the buffer gets
set with '\0'. This means that when the last value is parsed, the
trailing char is '\0'. Which, in turn, means that 'c' will always get a
value.

*Unless*, the buffer is changed in BPF_CGROUP_RUN_PROG_SYSCTL by an
external (to the kernel) program.

Are you running a BPF program on this hook? Do you see the same behavior
if you turn off BPF?

> case proc_do_large_bitmap() expects to be able to parse a second part of
> the input buffer. If there is no second part an unjustified -EINVAL will
> be returned.
> 
> Add check that left is non-zero before checking c, as proc_get_long()
> ensures that the passed left is non-zero, if a trailing character
> exists.
> 
> ---
I'll repeat the comments from the other reviewers. Careful with these
separators. When I used b4 to bring all this into my testing env, All
your trailers were ignored.

best

> When writing to /proc/sys/net/ipv4/ip_local_reserved_ports it is
> possible to receive an -EINVAL for a valid value.
> 
> This happens due to a check of a potentially uninitialized variable in
> the proc_do_large_bitmap() function, namely char c. To trigger this
> behavior the variable has to contain the later explicitly checked '-'
> char by chance.
> 
> In proc_do_large_bitmap() it is expected that the variable might be
> filled by the proc_get_long() function with the trailing character of
> the given input. But only if a trailing character exists within the
> passed size of the buffer.
> 
> If no trailing character is present we still do a c == '-' check. If the
> uninitialized variable contains this char the function continues
> parsing. It will now set err to -EINVAL in the next proc_get_long()
> call, as there is nothing more to parse.
> 
> proc_do_large_bitmap() passes left to the proc_get_long() call. left
> will only be non-zero, if a trailing character has been written.
> Therefore, checking that left is non-zero before accessing c fixes this
> problem.
> 
> The problem will only arise sporadically, as the variable must contain
> '-' by chance. On the affected system CONFIG_INIT_STACK_NONE=y was
> enabled. Further, when enabling eBPF tracing to dump contents of the
> stack the issue disappeared.
> 
> Fixes: 9f977fb7ae9d ("sysctl: add proc_do_large_bitmap")
> Signed-off-by: Marc Buerg <buermarc@googlemail.com>
> Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> ---
> Changes in v3:
> - Add Reviewed-by: Peter Seiderer <ps.report@gmx.net>
> - Re-include bug context into cover letter
> - Link to v2: https://lore.kernel.org/r/20260317-fix-uninitialized-variable-in-proc_do_large_bitmap-v2-1-6dfb1aefa287@googlemail.com
> 
> Changes in v2:
> - Drop initialization of c to 0
> - Include checking that left is non-zero before checking against c
> - Link to v1: https://lore.kernel.org/r/20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-v1-1-35ad2dddaf21@googlemail.com
> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 9d3a666ffde1..dd337a63da41 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1171,7 +1171,7 @@ int proc_do_large_bitmap(const struct ctl_table *table, int dir,
>  				left--;
>  			}
>  
> -			if (c == '-') {
> +			if (left && c == '-') {
>  				err = proc_get_long(&p, &left, &val_b,
>  						     &neg, tr_b, sizeof(tr_b),
>  						     &c);
> 
> ---
> base-commit: 80234b5ab240f52fa45d201e899e207b9265ef91
> change-id: 20260312-fix-uninitialized-variable-in-proc_do_large_bitmap-30c6ef4ac1c5
> 
> Best regards,
> -- 
> buermarc <buermarc@googlemail.com>
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[buermarc]

Hi Joel,

Thanks for the question.

On Mon, 23 Mar 2026 14:53:51 +0100, Joel Granados wrote:

> Here I have a question:
> 
> How is it possible that the buffer in proc_do_large_bitmap is not
> terminated with a '\0' when writing?
> 
> The proc_do_large_bitmap gets called from proc_sys_call_handler and
> before running the table->proc_handler call, the end of the buffer gets
> set with '\0'.

This is correct, the buffer will be terminated with a '\0'.

> trailing char is '\0'. Which, in turn, means that 'c' will always get a
> value.

I think this does not hold true. If the buffers is parsed till the end
len can be of the same value as size in proc_get_long(), which means we
do not set tr.

I will go through my current understanding for a
write(3</proc/sys/net/ipv4/ip_local_reserved_ports>, "123", 3) = 3
syscall. Hopefully that will make things a bit clearer, or show where I
am mistaken.

In proc_sys_call_handler() we call the function (in our case
proc_do_large_bitmap()) with the count based on the result of
iov_iter_count(), while ensuring that the buffer contains a trailing
'\0' char. So the buffer will contain '123\0', that is correct. The
passed count will does not change it will still be 3. I used bpftrace to
check my assumption as I don't know the iov_iter struct all that good
and it holds up.

    The trace code:
    sudo bpftrace -e '
    kprobe:proc_sys_call_handler
    {
        printf("Count: %lu\n", ((struct iov_iter *)arg1)->count);
    }

    kprobe:proc_do_large_bitmap
    {
        printf("Count: %lu\n", *(uint8 *)arg3);
    }'
    Attached 2 probes
    Count: 3
    Count: 3

Where the output is triggered by:
write(3</proc/sys/net/ipv4/ip_local_reserved_ports>, "123", 3) = 3

proc_do_large_bitmap() takes the given count as lenp and sets left to
the value lenp points to. left is then passed to proc_get_long(). This
means size in proc_get_long() is 3.

proc_get_long() calls strtoul_lenient(), which parses until it hits '\0'
and lets p point to that. See in proc_get_long():

	if (strtoul_lenient(p, &p, 0, val))
		return -EINVAL;

	len = p - tmp;

Setting len to 'p - tmp' means length is 3 in our example. In our case
size is also 3. As such:

    (len < *size) => (3 < 3) => false.

This means following branches will not be take. See in proc_get_long():

	if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len))
		return -EINVAL;

	if (tr && (len < *size))
		*tr = *p;

This means we do not set tr to what p points to. Also we do not do the
!memchr check. This branch would return -EINVAL if len < size holds true
and we do not contain an expected trailing character.

We can force the !memchr branch to return -EINVAL, by using
write(fd,'123\0',4). Here len is again 3, as we only read to '\0', but
the size is 4. This will, as expected, now return an -EINVAL, because
for the first proc_get_long() call we do not have '\0' in the set of
expected trailing characters.

See tr_a, and the first proc_get_long() call in proc_do_large_bitmap():

	char tr_a[] = { '-', ',', '\n' }, tr_b[] = { ',', '\n', 0 }, c;

        ...

        /* In case we stop parsing mid-number, we can reset */
        saved_left = left;
        err = proc_get_long(&p, &left, &val_a, &neg, tr_a,
                             sizeof(tr_a), &c);

For the second proc_get_long() call we use tr_b, and here using '\0' is
valid. The resulting behavior feels a bit unintuitive to me. I can
understand why based on the code and it likely has a some reason that I
just haven't figured out yet.

For example the following works:
write(3</proc/sys/net/ipv4/ip_local_reserved_ports>, "123-234\0", 8) = 8

But if we have same '\0' char at the end for the first part it fails:
write(3</proc/sys/net/ipv4/ip_local_reserved_ports>, "123\0", 4) = -1

> I'll repeat the comments from the other reviewers. Careful with these
> separators. When I used b4 to bring all this into my testing env, All
> your trailers were ignored.

I'll make sure to apply test output of b4 next time and check if
everything works locally as expected. Sorry for the mistake.

> *Unless*, the buffer is changed in BPF_CGROUP_RUN_PROG_SYSCTL by an
> external (to the kernel) program.
> 
> Are you running a BPF program on this hook? Do you see the same behavior
> if you turn off BPF?

I tried to see if I can access what c is on the stack to check if I
could see the '-' ascii value. I think I used the following snippet:

    # Print the specific byte where 'c' could be (e.g., RBP-1)
    # On x86_64, local chars could be at the end of the stack frame?
    sudo bpftrace -e 'kprobe:proc_do_large_bitmap {
        $c_val = *(u8 *)(reg("bp") - 1);
        printf("PID %d: c starts as 0x%02x (%c)\n", pid, $c_val, $c_val);
    }'

But I must admit this was just a shot into the blue. Yet, on the
affected host the -EINVAL results disappeared as long as the probe was
attached. My assumption was that the eBPF trace changed which value c
pointed to on the stack.

Best Regards,
Marc

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On Tue, Mar 24, 2026 at 12:46:23AM +0100, buermarc wrote:
> Hi Joel,
> 
> Thanks for the question.
> 
> On Mon, 23 Mar 2026 14:53:51 +0100, Joel Granados wrote:
> 
> > Here I have a question:
> > 
> > How is it possible that the buffer in proc_do_large_bitmap is not
> > terminated with a '\0' when writing?
> > 
> > The proc_do_large_bitmap gets called from proc_sys_call_handler and
> > before running the table->proc_handler call, the end of the buffer gets
> > set with '\0'.
> 
> This is correct, the buffer will be terminated with a '\0'.
> 
> > trailing char is '\0'. Which, in turn, means that 'c' will always get a
> > value.
> 
> I think this does not hold true. If the buffers is parsed till the end
> len can be of the same value as size in proc_get_long(), which means we
This is not the case:

1. len is calculated by doing len = p - tmp
2. tmp is the beginning of the buffer
3. p is the first character that is not a digit.

There will always be at least one ('\0') character that is not a digit.
So len will be less than size at least by one (could be more if the
string contains non digit chars). When it is parsed to the end,
len will be less than size by one.

All this holds unless the trailing '\0' is modified in some way in that
BPF call.

> do not set tr.
> 
> I will go through my current understanding for a
> write(3</proc/sys/net/ipv4/ip_local_reserved_ports>, "123", 3) = 3
> syscall. Hopefully that will make things a bit clearer, or show where I
> am mistaken.

TL;DR

> 
> In proc_sys_call_handler() we call the function (in our case
> proc_do_large_bitmap()) with the count based on the result of
> iov_iter_count(), while ensuring that the buffer contains a trailing
> '\0' char. So the buffer will contain '123\0', that is correct. The
> passed count will does not change it will still be 3. I used bpftrace to
> check my assumption as I don't know the iov_iter struct all that good
...
>     # On x86_64, local chars could be at the end of the stack frame?
>     sudo bpftrace -e 'kprobe:proc_do_large_bitmap {
>         $c_val = *(u8 *)(reg("bp") - 1);
>         printf("PID %d: c starts as 0x%02x (%c)\n", pid, $c_val, $c_val);
>     }'
> 
> But I must admit this was just a shot into the blue. Yet, on the
> affected host the -EINVAL results disappeared as long as the probe was
> attached. My assumption was that the eBPF trace changed which value c
> pointed to on the stack.
> 
> Best Regards,
> Marc

Best

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[buermarc]

From: Marc Buerg <buermarc@googlemail.com>

Hi,

On Tue, 24 Mar 2026 08:44:13 +0100 Joel Granados, wrote:

> 1. len is calculated by doing len = p - tmp
> 2. tmp is the beginning of the buffer
> 3. p is the first character that is not a digit.
>
> There will always be at least one ('\0') character that is not a digit.

Yes.

> So len will be less than size at least by one (could be more if the
> string contains non digit chars). When it is parsed to the end,
> len will be less than size by one.

I don't think this is the case. See below.

> TL;DR

I'll try to make it shorter. size is the length of the user buffer, even
after the copy to the kernel buffer. The added '\0' does not increment
size in proc_sys_call_handler(). Following the example for
write(fd,"123",3). In the beginning of proc_get_long():

size = 3

@:012 3
-------
 "123\0"
  |
 tmp=@0
  |
  p=@0

After strtoul_lenient():

@:012 3
-------
 "123\0"
  |   |
 tmp=@0
      |
      p=@3

@3 - @0 = 3
len = 3

(len < size) => (3 < 3) => false

And size must be 3, proc_sys_call_handler() does not increment it. If we
provide something like write(fd,"123\0",4) size will be 4, but now we will
return -EINVAL.

If len < size is true, we call memchk(). memchk will not find a trailing
character as p points to '\0'. This means in this case we would return
-EINVAL for the first proc_get_long() call in the while loop. Reason is
'\0' is not in tr_a and therefore not a valid delimiter for the first
proc_get_long() call.

I hope this get's my point across.

> All this holds unless the trailing '\0' is modified in some way in that
> BPF call.

To my knowledge there was no bpf tracing enabled on the affected host at
the time that the behavior was observed.

Finally: the fix which adds a check for left introduces a regression.
"123-" is wrongly accepted. Not sure if you saw my other message. It
should not be used.

Best Regards,
Marc

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

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

On Tue, Mar 24, 2026 at 11:36:40PM +0100, buermarc wrote:
> From: Marc Buerg <buermarc@googlemail.com>
> 
> Hi,
> 
> On Tue, 24 Mar 2026 08:44:13 +0100 Joel Granados, wrote:
> 
> > 1. len is calculated by doing len = p - tmp
> > 2. tmp is the beginning of the buffer
> > 3. p is the first character that is not a digit.
> >
> > There will always be at least one ('\0') character that is not a digit.
> 
> Yes.
> 
> > So len will be less than size at least by one (could be more if the
> > string contains non digit chars). When it is parsed to the end,
> > len will be less than size by one.
> 
> I don't think this is the case. See below.
> 
> > TL;DR
> 
> I'll try to make it shorter. size is the length of the user buffer, even
> after the copy to the kernel buffer. The added '\0' does not increment
> size in proc_sys_call_handler(). Following the example for
> write(fd,"123",3). In the beginning of proc_get_long():
> 
> size = 3
> 
> @:012 3
> -------
>  "123\0"
>   |
>  tmp=@0
>   |
>   p=@0
> 
> After strtoul_lenient():
> 
> @:012 3
> -------
>  "123\0"
>   |   |
>  tmp=@0
>       |
>       p=@3
> 
> @3 - @0 = 3
> len = 3
> 
> (len < size) => (3 < 3) => false

I see it now! I needed to pass '-n' to echo. I was testing with
write(fd, "123\n" 4) instead of write(fd, "123", 3)

> 
> And size must be 3, proc_sys_call_handler() does not increment it. If we
> provide something like write(fd,"123\0",4) size will be 4, but now we will
> return -EINVAL.
Agreed.

> 
> If len < size is true, we call memchk(). memchk will not find a trailing
> character as p points to '\0'. This means in this case we would return
> -EINVAL for the first proc_get_long() call in the while loop. Reason is
> '\0' is not in tr_a and therefore not a valid delimiter for the first
> proc_get_long() call.
> 
> I hope this get's my point across.
It does. Thx for insisting :)

> 
> > All this holds unless the trailing '\0' is modified in some way in that
> > BPF call.
> 
> To my knowledge there was no bpf tracing enabled on the affected host at
> the time that the behavior was observed.
Indeed. I can reproduce without BPF

> 
> Finally: the fix which adds a check for left introduces a regression.
> "123-" is wrongly accepted. Not sure if you saw my other message. It
> should not be used.
I saw that, thx.

I think the correct fix is your original version. Init 'c' to zero only.
Please resend it without the '---' so I can pull your trailers properly.

We don't check 'left' in the c == '-' condition because it will
incorrectly accept strings that end in '-' (e.g. "123-"). In other
words, we need to enter that branch to fail as expected.

> 
> Best Regards,
> Marc

Thx

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marc Buerg]

Hi Joel,

Thanks again for the review and question.

On Wed, 25 Mar 2026 11:07:23 +0100, Joel Joel Granados, wrote:

> I think the correct fix is your original version. Init 'c' to zero only.
> Please resend it without the '---' so I can pull your trailers properly.

Makes sense, will do.

Best Regards,
Marc