[PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

Today, proc_do_large_bitmap() truncates a large write input buffer
to PAGE_SIZE - 1, which may result in misparsed numbers at the
(truncated) end of the buffer.  Further, it fails to notify the caller
that the buffer was truncated, so it doesn't get called iteratively
to finish the entire input buffer.

Tell the caller if there's more work to do by adding the skipped
amount back to left/*lenp before returning.

To fix the misparsing, reset the position if we have completely
consumed a truncated buffer (or if just one char is left, which
may be a "-" in a range), and ask the caller to come back for
more.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ba4d9e85feb8..970a96659809 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3089,9 +3089,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 
 	if (write) {
 		char *kbuf, *p;
+		size_t skipped = 0;
 
-		if (left > PAGE_SIZE - 1)
+		if (left > PAGE_SIZE - 1) {
 			left = PAGE_SIZE - 1;
+			/* How much of the buffer we'll skip this pass */
+			skipped = *lenp - left;
+		}
 
 		p = kbuf = memdup_user_nul(buffer, left);
 		if (IS_ERR(kbuf))
@@ -3108,9 +3112,22 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 		while (!err && left) {
 			unsigned long val_a, val_b;
 			bool neg;
+			size_t saved_left;
 
+			/* 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);
+			/*
+			 * If we consumed the entirety of a truncated buffer or
+			 * only one char is left (may be a "-"), then stop here,
+			 * reset, & come back for more.
+			 */
+			if ((left <= 1) && skipped) {
+				left = saved_left;
+				break;
+			}
+
 			if (err)
 				break;
 			if (val_a >= bitmap_len || neg) {
@@ -3128,6 +3145,15 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 				err = proc_get_long(&p, &left, &val_b,
 						     &neg, tr_b, sizeof(tr_b),
 						     &c);
+				/*
+				 * If we consumed all of a truncated buffer or
+				 * then stop here, reset, & come back for more.
+				 */
+				if (!left && skipped) {
+					left = saved_left;
+					break;
+				}
+
 				if (err)
 					break;
 				if (val_b >= bitmap_len || neg ||
@@ -3146,6 +3172,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 			proc_skip_char(&p, &left, '\n');
 		}
 		kfree(kbuf);
+		left += skipped;
 	} else {
 		unsigned long bit_a, bit_b = 0;
 

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

Here's a pretty hacky test script to test this code via
ip_local_reserved_ports

-----

#!/bin/bash

# Randomly construct well-formed (sequential, non-overlapping)
# input for ip_local_reserved_ports, feed it to the sysctl,
# then read it back and check for differences.

# Port range to use
PORT_START=1024
PORT_STOP=32768

# Total length of ports string to use
LENGTH=$((4096+$((RANDOM % 16384))))

# String containing our list of ports
PORTS=$PORT_START

# Try 1000 times
for I in `seq 1 1000`; do
	
	# build up the string
	while true; do
		# Make sure it's discontiguous, skip ahead at least 2
		SKIP=$((2 + RANDOM % 10))
		PORT_START=$((PORT_START + SKIP))
	
		if [ "$PORT_START" -ge "$PORT_STOP" ]; then
			break;
		fi
	
		# 14856-14863,14861
		# Add a range, or a single port
		USERANGE=$((RANDOM % 2))
	
		if [ "$USERANGE" -eq "1" ]; then
			RANGE_START=$PORT_START
			RANGE_LEN=$((1 + RANDOM % 10))
			RANGE_END=$((RANGE_START + RANGE_LEN))
			PORTS="${PORTS},${RANGE_START}-${RANGE_END}"
			# Break out if we've done enough
			if [ "$RANGE_END" -eq "$PORT_STOP" ]; then
				break;
			fi
			PORT_START=$RANGE_END
		else
			PORTS="${PORTS},${PORT_START}"
		fi
	
		if [ "${#PORTS}" -gt "$LENGTH" ]; then
			break;
		fi
	
	done
	
	# See if we get out what we put in
	echo "Trial $I"
	echo $PORTS > port_list
	cat port_list > /proc/sys/net/ipv4/ip_local_reserved_ports || break
	cat /proc/sys/net/ipv4/ip_local_reserved_ports > port_list_out
	diff -uq port_list port_list_out || break
	
done

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Luis Chamberlain]

On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
> Here's a pretty hacky test script to test this code via
> ip_local_reserved_ports

Thanks Eric!

So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
if we wanted to stress test it with a selftest it could break other self
tests or change the system behaviour. Because of this we have now have
lib/test_sysctl.c, and we test this with the script:

tools/testing/selftests/sysctl/sysctl.sh

Any chance you can extend lib/test_sysctl.c with a new respective bitmap
knob, and add a respective test? This will ensure we don't regress
later. 0-day runs sysctl.sh so it should catch any regressions in the
future.

If you can think of other ways to test the knob that would be great too.

  Luis

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

On 2/21/19 9:18 AM, Luis Chamberlain wrote:
> On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
>> Here's a pretty hacky test script to test this code via
>> ip_local_reserved_ports
> 
> Thanks Eric!
> 
> So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
> if we wanted to stress test it with a selftest it could break other self
> tests or change the system behaviour. Because of this we have now have
> lib/test_sysctl.c, and we test this with the script:
> 
> tools/testing/selftests/sysctl/sysctl.sh
> 
> Any chance you can extend lib/test_sysctl.c with a new respective bitmap
> knob,

Done

> and add a respective test? This will ensure we don't regress
> later. 0-day runs sysctl.sh so it should catch any regressions in the
> future.

As you know, learning somebody else's test harness infra is a PITA. ;)
Can you find me off-list and give me a hand with this?

Thanks,
-Eric

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Luis Chamberlain]

On Thu, Feb 21, 2019 at 11:47:49AM -0600, Eric Sandeen wrote:
> On 2/21/19 9:18 AM, Luis Chamberlain wrote:
> > On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
> >> Here's a pretty hacky test script to test this code via
> >> ip_local_reserved_ports
> > 
> > Thanks Eric!
> > 
> > So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
> > if we wanted to stress test it with a selftest it could break other self
> > tests or change the system behaviour. Because of this we have now have
> > lib/test_sysctl.c, and we test this with the script:
> > 
> > tools/testing/selftests/sysctl/sysctl.sh
> > 
> > Any chance you can extend lib/test_sysctl.c with a new respective bitmap
> > knob,
> 
> Done

Thanks!

> > and add a respective test? This will ensure we don't regress
> > later. 0-day runs sysctl.sh so it should catch any regressions in the
> > future.
> 
> As you know, learning somebody else's test harness infra is a PITA. ;)
> Can you find me off-list and give me a hand with this?

Sure, its actually quite simple, just as root, run the script.

  Luis

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

On 2/21/19 11:52 AM, Luis Chamberlain wrote:
> On Thu, Feb 21, 2019 at 11:47:49AM -0600, Eric Sandeen wrote:
>> On 2/21/19 9:18 AM, Luis Chamberlain wrote:
>>> On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
>>>> Here's a pretty hacky test script to test this code via
>>>> ip_local_reserved_ports
>>>
>>> Thanks Eric!
>>>
>>> So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
>>> if we wanted to stress test it with a selftest it could break other self
>>> tests or change the system behaviour. Because of this we have now have
>>> lib/test_sysctl.c, and we test this with the script:
>>>
>>> tools/testing/selftests/sysctl/sysctl.sh
>>>
>>> Any chance you can extend lib/test_sysctl.c with a new respective bitmap
>>> knob,
>>
>> Done
> 
> Thanks!
> 
>>> and add a respective test? This will ensure we don't regress
>>> later. 0-day runs sysctl.sh so it should catch any regressions in the
>>> future.
>>
>> As you know, learning somebody else's test harness infra is a PITA. ;)
>> Can you find me off-list and give me a hand with this?
> 
> Sure, its actually quite simple, just as root, run the script.

Running it looks easy.  I'd like to know about how to extend it.

Thanks,
-Eric

[PATCH] sysctl: add proc_do_large_bitmap test node

[Eric Sandeen]

Add a test node for proc_do_large_bitmap to the test_sysctl.c
infrastructure.  It's sized the same as the one existing user.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 3dd801c1c85b..1263be4ebfaf 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -47,6 +47,9 @@ struct test_sysctl_data {
 	unsigned int uint_0001;
 
 	char string_0001[65];
+
+#define SYSCTL_TEST_BITMAP_SIZE	65536
+	unsigned long *bitmap_0001;
 };
 
 static struct test_sysctl_data test_data = {
@@ -102,6 +106,13 @@ static struct ctl_table test_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dostring,
 	},
+	{
+		.procname	= "bitmap_0001",
+		.data		= &test_data.bitmap_0001,
+		.maxlen		= SYSCTL_TEST_BITMAP_SIZE,
+		.mode		= 0644,
+		.proc_handler	= proc_do_large_bitmap,
+	},
 	{ }
 };
 
@@ -129,15 +140,21 @@ static struct ctl_table_header *test_sysctl_header;
 
 static int __init test_sysctl_init(void)
 {
+	test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL);
+	if (!test_data.bitmap_0001)
+		return -ENOMEM;
 	test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
-	if (!test_sysctl_header)
+	if (!test_sysctl_header) {
+		kfree(test_data.bitmap_0001);
 		return -ENOMEM;
+	}
 	return 0;
 }
 late_initcall(test_sysctl_init);
 
 static void __exit test_sysctl_exit(void)
 {
+	kfree(test_data.bitmap_0001);
 	if (test_sysctl_header)
 		unregister_sysctl_table(test_sysctl_header);
 }

Re: [PATCH] sysctl: add proc_do_large_bitmap test node

[Kees Cook]

On Thu, Feb 21, 2019 at 9:45 AM Eric Sandeen <sandeen@sandeen.net> wrote:
>
> Add a test node for proc_do_large_bitmap to the test_sysctl.c
> infrastructure.  It's sized the same as the one existing user.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>
> diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
> index 3dd801c1c85b..1263be4ebfaf 100644
> --- a/lib/test_sysctl.c
> +++ b/lib/test_sysctl.c
> @@ -47,6 +47,9 @@ struct test_sysctl_data {
>         unsigned int uint_0001;
>
>         char string_0001[65];
> +
> +#define SYSCTL_TEST_BITMAP_SIZE        65536
> +       unsigned long *bitmap_0001;
>  };
>
>  static struct test_sysctl_data test_data = {
> @@ -102,6 +106,13 @@ static struct ctl_table test_table[] = {
>                 .mode           = 0644,
>                 .proc_handler   = proc_dostring,
>         },
> +       {
> +               .procname       = "bitmap_0001",
> +               .data           = &test_data.bitmap_0001,
> +               .maxlen         = SYSCTL_TEST_BITMAP_SIZE,
> +               .mode           = 0644,
> +               .proc_handler   = proc_do_large_bitmap,
> +       },
>         { }
>  };
>
> @@ -129,15 +140,21 @@ static struct ctl_table_header *test_sysctl_header;
>
>  static int __init test_sysctl_init(void)
>  {
> +       test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL);
> +       if (!test_data.bitmap_0001)
> +               return -ENOMEM;
>         test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
> -       if (!test_sysctl_header)
> +       if (!test_sysctl_header) {
> +               kfree(test_data.bitmap_0001);
>                 return -ENOMEM;
> +       }
>         return 0;
>  }
>  late_initcall(test_sysctl_init);
>
>  static void __exit test_sysctl_exit(void)
>  {
> +       kfree(test_data.bitmap_0001);
>         if (test_sysctl_header)
>                 unregister_sysctl_table(test_sysctl_header);
>  }
>


-- 
Kees Cook

[PATCH] test_sysctl: add proc_do_large_bitmap test function

[Eric Sandeen]

Add test to build up bitmap range string and test the bitmap
proc handler.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

nb: test_modprobe & load_req_mod fail for me before we ever
get to this test, but commenting them out, my test runs as expected.
I'm new to this script, so careful review would be wise. ;)

Thanks,
-Eric

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 584eb8ea780a..c710b09e2d69 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -37,6 +37,7 @@ ALL_TESTS="$ALL_TESTS 0002:1:1"
 ALL_TESTS="$ALL_TESTS 0003:1:1"
 ALL_TESTS="$ALL_TESTS 0004:1:1"
 ALL_TESTS="$ALL_TESTS 0005:3:1"
+ALL_TESTS="$ALL_TESTS 0006:3:1"
 
 test_modprobe()
 {
@@ -149,6 +150,9 @@ reset_vals()
 		string_0001)
 			VAL="(none)"
 			;;
+		bitmap_0001)
+			VAL=""
+			;;
 		*)
 			;;
 	esac
@@ -548,6 +552,47 @@ run_stringtests()
 	test_rc
 }
 
+run_bitmaptest() {
+	# Total length of bitmaps string to use, a bit under
+	# the maximum input size of the test node
+	LENGTH=$((RANDOM % 65000))
+
+	# First bit to set
+	BIT=$((RANDOM % 1024))
+
+	# String containing our list of bits to set
+	TEST_STR=$BIT
+
+	# build up the string
+	while [ "${#TEST_STR}" -le "$LENGTH" ]; do
+		# Make sure next entry is discontiguous,
+		# skip ahead at least 2
+		BIT=$((BIT + $((2 + RANDOM % 10))))
+
+		# Add new bit to the list
+		TEST_STR="${TEST_STR},${BIT}"
+
+		# Randomly make it a range
+		if [ "$((RANDOM % 2))" -eq "1" ]; then
+			RANGE_END=$((BIT + $((1 + RANDOM % 10))))
+			TEST_STR="${TEST_STR}-${RANGE_END}"
+			BIT=$RANGE_END
+		fi
+	done
+
+	echo -n "Checking bitmap handler... "
+	set_orig
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
 sysctl_test_0001()
 {
 	TARGET="${SYSCTL}/int_0001"
@@ -605,6 +650,14 @@ sysctl_test_0005()
 	run_limit_digit_int_array
 }
 
+sysctl_test_0006()
+{
+	TARGET="${SYSCTL}/bitmap_0001"
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	run_bitmaptest
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -618,6 +671,7 @@ list_tests()
 	echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
 	echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
 	echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
+	echo "0006 x $(get_test_count 0006) - tests proc_do_large_bitmap"
 }
 
 test_reqs

Re: [PATCH] test_sysctl: add proc_do_large_bitmap test function

[Eric Sandeen]

On 2/21/19 12:43 PM, Eric Sandeen wrote:
> Add test to build up bitmap range string and test the bitmap
> proc handler.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> nb: test_modprobe & load_req_mod fail for me before we ever
> get to this test, but commenting them out, my test runs as expected.
> I'm new to this script, so careful review would be wise. ;)
> 
> Thanks,
> -Eric

Gah, running this in different ways, it's not doing the right thing.
Hang on, V2 pending...

[PATCH V2] test_sysctl: add proc_do_large_bitmap test function

[Eric Sandeen]

Add test to build up bitmap range string and test the bitmap
proc handler.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: set rc=0 for test success

however this still fails indeterminately for me.  Debugging, if I
save off the test write string and the read string, re-writing it to
the handler works fine.  My standalone test also works fine for 1000
iterations.  So I don't know what's going on here.

nb: test_modprobe & load_req_mod fail for me before we ever
get to this test, but commenting them out, my test runs as expected.
I'm new to this script, so careful review would be wise. 

Thanks,
-Eric


diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 584eb8ea780a..e531074f94bd 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -37,6 +37,7 @@ ALL_TESTS="$ALL_TESTS 0002:1:1"
 ALL_TESTS="$ALL_TESTS 0003:1:1"
 ALL_TESTS="$ALL_TESTS 0004:1:1"
 ALL_TESTS="$ALL_TESTS 0005:3:1"
+ALL_TESTS="$ALL_TESTS 0006:3:1"
 
 test_modprobe()
 {
@@ -149,6 +150,9 @@ reset_vals()
 		string_0001)
 			VAL="(none)"
 			;;
+		bitmap_0001)
+			VAL=""
+			;;
 		*)
 			;;
 	esac
@@ -548,6 +552,48 @@ run_stringtests()
 	test_rc
 }
 
+run_bitmaptest() {
+	# Total length of bitmaps string to use, a bit under
+	# the maximum input size of the test node
+	LENGTH=$((RANDOM % 65000))
+
+	# First bit to set
+	BIT=$((RANDOM % 1024))
+
+	# String containing our list of bits to set
+	TEST_STR=$BIT
+
+	# build up the string
+	while [ "${#TEST_STR}" -le "$LENGTH" ]; do
+		# Make sure next entry is discontiguous,
+		# skip ahead at least 2
+		BIT=$((BIT + $((2 + RANDOM % 10))))
+
+		# Add new bit to the list
+		TEST_STR="${TEST_STR},${BIT}"
+
+		# Randomly make it a range
+		if [ "$((RANDOM % 2))" -eq "1" ]; then
+			RANGE_END=$((BIT + $((1 + RANDOM % 10))))
+			TEST_STR="${TEST_STR}-${RANGE_END}"
+			BIT=$RANGE_END
+		fi
+	done
+
+	echo -n "Checking bitmap handler... "
+	echo $TEST_STR > /tmp/test_str
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+		rc=0
+	fi
+	test_rc
+}
+
 sysctl_test_0001()
 {
 	TARGET="${SYSCTL}/int_0001"
@@ -605,6 +651,14 @@ sysctl_test_0005()
 	run_limit_digit_int_array
 }
 
+sysctl_test_0006()
+{
+	TARGET="${SYSCTL}/bitmap_0001"
+	reset_vals
+	ORIG=""
+	run_bitmaptest
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -618,6 +672,7 @@ list_tests()
 	echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
 	echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
 	echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
+	echo "0006 x $(get_test_count 0006) - tests proc_do_large_bitmap"
 }
 
 test_reqs

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

On 2/20/19 5:32 PM, Eric Sandeen wrote:
> Today, proc_do_large_bitmap() truncates a large write input buffer
> to PAGE_SIZE - 1, which may result in misparsed numbers at the
> (truncated) end of the buffer.  Further, it fails to notify the caller
> that the buffer was truncated, so it doesn't get called iteratively
> to finish the entire input buffer.
> 
> Tell the caller if there's more work to do by adding the skipped
> amount back to left/*lenp before returning.
> 
> To fix the misparsing, reset the position if we have completely
> consumed a truncated buffer (or if just one char is left, which
> may be a "-" in a range), and ask the caller to come back for
> more.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Would be nice to fix this bug.  I submitted the test node patch as well
as an attempt to integrate it into the test harness, though there's
wonkiness there still, and I could use more experienced eyes.

Can we move this forward?

Thanks,
-Eric

> ---
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ba4d9e85feb8..970a96659809 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3089,9 +3089,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  
>  	if (write) {
>  		char *kbuf, *p;
> +		size_t skipped = 0;
>  
> -		if (left > PAGE_SIZE - 1)
> +		if (left > PAGE_SIZE - 1) {
>  			left = PAGE_SIZE - 1;
> +			/* How much of the buffer we'll skip this pass */
> +			skipped = *lenp - left;
> +		}
>  
>  		p = kbuf = memdup_user_nul(buffer, left);
>  		if (IS_ERR(kbuf))
> @@ -3108,9 +3112,22 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  		while (!err && left) {
>  			unsigned long val_a, val_b;
>  			bool neg;
> +			size_t saved_left;
>  
> +			/* 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);
> +			/*
> +			 * If we consumed the entirety of a truncated buffer or
> +			 * only one char is left (may be a "-"), then stop here,
> +			 * reset, & come back for more.
> +			 */
> +			if ((left <= 1) && skipped) {
> +				left = saved_left;
> +				break;
> +			}
> +
>  			if (err)
>  				break;
>  			if (val_a >= bitmap_len || neg) {
> @@ -3128,6 +3145,15 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  				err = proc_get_long(&p, &left, &val_b,
>  						     &neg, tr_b, sizeof(tr_b),
>  						     &c);
> +				/*
> +				 * If we consumed all of a truncated buffer or
> +				 * then stop here, reset, & come back for more.
> +				 */
> +				if (!left && skipped) {
> +					left = saved_left;
> +					break;
> +				}
> +
>  				if (err)
>  					break;
>  				if (val_b >= bitmap_len || neg ||
> @@ -3146,6 +3172,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  			proc_skip_char(&p, &left, '\n');
>  		}
>  		kfree(kbuf);
> +		left += skipped;
>  	} else {
>  		unsigned long bit_a, bit_b = 0;
>  
> 
> 

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Luis Chamberlain]

On Mon, Mar 04, 2019 at 10:43:09PM -0600, Eric Sandeen wrote:
> On 2/20/19 5:32 PM, Eric Sandeen wrote:
> > Today, proc_do_large_bitmap() truncates a large write input buffer
> > to PAGE_SIZE - 1, which may result in misparsed numbers at the
> > (truncated) end of the buffer.  Further, it fails to notify the caller
> > that the buffer was truncated, so it doesn't get called iteratively
> > to finish the entire input buffer.
> > 
> > Tell the caller if there's more work to do by adding the skipped
> > amount back to left/*lenp before returning.
> > 
> > To fix the misparsing, reset the position if we have completely
> > consumed a truncated buffer (or if just one char is left, which
> > may be a "-" in a range), and ask the caller to come back for
> > more.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Would be nice to fix this bug.  I submitted the test node patch as well
> as an attempt to integrate it into the test harness, though there's
> wonkiness there still, and I could use more experienced eyes.

Sorry for the delay.

I'm rolling these changes in with some minor adjustments, can you send
me your respective lib/test_sysctl.c changes? I don't see that they had
been sent.

  Luis

Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Luis Chamberlain]

On Tue, Mar 19, 2019 at 9:30 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> can you send
> me your respective lib/test_sysctl.c changes? I don't see that they had
> been sent.

Never mind, I see it now, will roll all this in.

 Luis