[LTP] [PATCH] sysctl/sysctl02: Add new regression test for overflow file-max

[LTP] [PATCH] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

On upstream kernel, before commit[1], the max value in proc_get_long based on
the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
but accepts values such as 18446744073709551616 (20 chars). But we should reject all
because both are overflows. After this commit,the permitted max value is 2^64-1.

Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
commit sets the max and min value for file-max. After this commit,the permitted max
value is 2^63-1.

Unfortunately, commit[2] introduced a new bug because the minimum value points at
the global 'zero' variable which is an int. It may crash system. This bug has been fixed
bt commit[3].

I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
[3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/commands                      | 1 +
 testcases/commands/sysctl/sysctl01.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/runtest/commands b/runtest/commands
index ac15e8b23..1870c4209 100644
--- a/runtest/commands
+++ b/runtest/commands
@@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
 gdb01_sh gdb01.sh
 unshare01_sh unshare01.sh
 sysctl01_sh sysctl01.sh
+sysctl02_sh sysctl02.sh
diff --git a/testcases/commands/sysctl/sysctl01.sh b/testcases/commands/sysctl/sysctl01.sh
index 7522da53d..bf89a6d8d 100755
--- a/testcases/commands/sysctl/sysctl01.sh
+++ b/testcases/commands/sysctl/sysctl01.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-
+#
 # Copyright (c) 2018 FUJITSU LIMITED. All rights reserved.
 # Author: Xiao Yang <yangx.jy@cn.fujitsu.com>
 #
-- 
2.18.1

[LTP] [PATCH] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

Hi all

  Please ingore this email. I will send a v2 patch. :-[ 

Thanks.

> On upstream kernel, before commit[1], the max value in proc_get_long based on
> the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
> but accepts values such as 18446744073709551616 (20 chars). But we should reject all
> because both are overflows. After this commit,the permitted max value is 2^64-1.
>
> Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
> /proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
> commit sets the max and min value for file-max. After this commit,the permitted max
> value is 2^63-1.
>
> Unfortunately, commit[2] introduced a new bug because the minimum value points at
> the global 'zero' variable which is an int. It may crash system. This bug has been fixed
> bt commit[3].
>
> I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.
>
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
> [3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972
>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  runtest/commands                      | 1 +
>  testcases/commands/sysctl/sysctl01.sh | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/runtest/commands b/runtest/commands
> index ac15e8b23..1870c4209 100644
> --- a/runtest/commands
> +++ b/runtest/commands
> @@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
>  gdb01_sh gdb01.sh
>  unshare01_sh unshare01.sh
>  sysctl01_sh sysctl01.sh
> +sysctl02_sh sysctl02.sh
> diff --git a/testcases/commands/sysctl/sysctl01.sh b/testcases/commands/sysctl/sysctl01.sh
> index 7522da53d..bf89a6d8d 100755
> --- a/testcases/commands/sysctl/sysctl01.sh
> +++ b/testcases/commands/sysctl/sysctl01.sh
> @@ -1,5 +1,5 @@
>  #!/bin/sh
> -
> +#
>  # Copyright (c) 2018 FUJITSU LIMITED. All rights reserved.
>  # Author: Xiao Yang <yangx.jy@cn.fujitsu.com>
>  #

[LTP] [PATCH v2] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

On upstream kernel, before commit[1], the max value in proc_get_long based on
the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
but accepts values such as 18446744073709551616 (20 chars). But we should reject all
because both are overflows. After this commit,the permitted max value is 2^64-1.

Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
commit sets the max and min value for file-max. After this commit,the permitted max
value is 2^63-1.

Unfortunately, commit[2] introduced a new bug because the minimum value points at
the global 'zero' variable which is an int. It may crash system. This bug has been fixed
bt commit[3].

I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
[3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/commands                      |  1 +
 testcases/commands/sysctl/sysctl02.sh | 88 +++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)
 create mode 100755 testcases/commands/sysctl/sysctl02.sh

diff --git a/runtest/commands b/runtest/commands
index ac15e8b23..1870c4209 100644
--- a/runtest/commands
+++ b/runtest/commands
@@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
 gdb01_sh gdb01.sh
 unshare01_sh unshare01.sh
 sysctl01_sh sysctl01.sh
+sysctl02_sh sysctl02.sh
diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
new file mode 100755
index 000000000..9ca9d0609
--- /dev/null
+++ b/testcases/commands/sysctl/sysctl02.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2019 FUJITSU LIMITED. All rights reserved.
+# Author: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
+#
+# Description:
+# This is a regression test for handling overflow for file-max.
+#
+# when writing 2^64 to /proc/sys/fs/file-max. It will overflow
+# and be set to 0. It crash system quickly.
+#
+# The kernel bug has been fixed in kernel:
+# '7f2923c4f' (sysctl: handle overflow in proc_get_long)
+# the permitted max value is  2^64-1.
+# '32a5ad9c2' (sysctl: handle overflow for file-max)
+# the permitted max value is 2^63-1
+#
+# After merged this patchset, if we exceed the max value, it will
+# keep old value. Unfortunately, it introudced a new bug when set it
+# to 0 and it will lead to system crash.
+# This bug has been fixed by commit 9002b2146
+# (kernel/sysctl.c: fix out-of-bounds access when setting file-max)
+
+TST_TESTFUNC=do_test
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+TST_CNT=4
+TST_NEEDS_ROOT=1
+TST_NEEDS_CMDS="sysctl"
+dir="/proc/sys/fs/"
+name="file-max"
+orig_value=200000
+check1=18446744073709551616
+check2=18446744073709551615
+check3=9223372036854775808
+check4=0
+
+. tst_test.sh
+
+setup()
+{
+	[ ! -f "$dir""$name" ] && tst_brk TCONF \
+		"$name was not supported"
+	orig_value=$(cat "$dir""$name")
+}
+
+do_test()
+{
+	case $1 in
+	1)sysctl_test_overflow ${check1};;
+	2)sysctl_test_overflow ${check2};;
+	3)sysctl_test_overflow ${check3};;
+	4)sysctl_test_zero ${check4};;
+	esac
+}
+
+sysctl_test_overflow()
+{
+	local old_value=$(cat "$dir""$name")
+
+	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
+
+	local test_value=$(cat "$dir""$name")
+
+	echo ${test_value} |grep -q ${old_value}
+	if [ $? -eq 0 ]; then
+		tst_res TPASS "file-max overflow, reject it and keep old value."
+	else
+		tst_res TFAIL "file-max overflow and set it to ${test_value}."
+	fi
+	cleanup
+}
+
+sysctl_test_zero()
+{
+	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
+	tst_res TINFO "if it doesn't report TPASS after 60s sleep, system crashes"
+	sleep 60
+	tst_res TPASS "file-max is set 0 and doesn't crash"
+}
+
+cleanup()
+{
+	sysctl -w "fs.""$name"=${orig_value} >/dev/null 2>&1
+}
+
+tst_run
-- 
2.18.1

[LTP] [PATCH v2] sysctl/sysctl02: Add new regression test for overflow file-max

[Cyril Hrubis]

Hi!
> Unfortunately, commit[2] introduced a new bug because the minimum value points at
> the global 'zero' variable which is an int. It may crash system. This bug has been fixed
> bt commit[3].
> 
> I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.
> 
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
> [3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  runtest/commands                      |  1 +
>  testcases/commands/sysctl/sysctl02.sh | 88 +++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>  create mode 100755 testcases/commands/sysctl/sysctl02.sh
> 
> diff --git a/runtest/commands b/runtest/commands
> index ac15e8b23..1870c4209 100644
> --- a/runtest/commands
> +++ b/runtest/commands
> @@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
>  gdb01_sh gdb01.sh
>  unshare01_sh unshare01.sh
>  sysctl01_sh sysctl01.sh
> +sysctl02_sh sysctl02.sh
> diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
> new file mode 100755
> index 000000000..9ca9d0609
> --- /dev/null
> +++ b/testcases/commands/sysctl/sysctl02.sh
> @@ -0,0 +1,88 @@
> +#!/bin/sh
> +
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2019 FUJITSU LIMITED. All rights reserved.
> +# Author: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
> +#
> +# Description:
> +# This is a regression test for handling overflow for file-max.
> +#
> +# when writing 2^64 to /proc/sys/fs/file-max. It will overflow
> +# and be set to 0. It crash system quickly.
> +#
> +# The kernel bug has been fixed in kernel:
> +# '7f2923c4f' (sysctl: handle overflow in proc_get_long)
> +# the permitted max value is  2^64-1.
> +# '32a5ad9c2' (sysctl: handle overflow for file-max)
> +# the permitted max value is 2^63-1
> +#
> +# After merged this patchset, if we exceed the max value, it will
> +# keep old value. Unfortunately, it introudced a new bug when set it
> +# to 0 and it will lead to system crash.
> +# This bug has been fixed by commit 9002b2146
> +# (kernel/sysctl.c: fix out-of-bounds access when setting file-max)
> +
> +TST_TESTFUNC=do_test
> +TST_SETUP=setup
> +TST_CLEANUP=cleanup
> +TST_CNT=4
> +TST_NEEDS_ROOT=1
> +TST_NEEDS_CMDS="sysctl"
> +dir="/proc/sys/fs/"
> +name="file-max"
> +orig_value=200000
> +check1=18446744073709551616
> +check2=18446744073709551615
> +check3=9223372036854775808
> +check4=0
> +
> +. tst_test.sh
> +
> +setup()
> +{
> +	[ ! -f "$dir""$name" ] && tst_brk TCONF \
> +		"$name was not supported"
> +	orig_value=$(cat "$dir""$name")
> +}
> +
> +do_test()
> +{
> +	case $1 in
> +	1)sysctl_test_overflow ${check1};;
> +	2)sysctl_test_overflow ${check2};;
> +	3)sysctl_test_overflow ${check3};;
> +	4)sysctl_test_zero ${check4};;

There is no point in having the numbers in check variables if we do case
here, we can just pass it here.

> +	esac
> +}
> +
> +sysctl_test_overflow()
> +{
> +	local old_value=$(cat "$dir""$name")
> +
> +	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
> +
> +	local test_value=$(cat "$dir""$name")
> +
> +	echo ${test_value} |grep -q ${old_value}
> +	if [ $? -eq 0 ]; then
> +		tst_res TPASS "file-max overflow, reject it and keep old value."
> +	else
> +		tst_res TFAIL "file-max overflow and set it to ${test_value}."
> +	fi
> +	cleanup
> +}
> +
> +sysctl_test_zero()
> +{
> +	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
> +	tst_res TINFO "if it doesn't report TPASS after 60s sleep, system crashes"
> +	sleep 60

What happens on the buggy kernel here?

Does it crash reliably?

It looks to me that reproducing this bug without KASAN enabled kernel
would be really hard or even impossible, therefore the sleep here does
not matter at all.

> +	tst_res TPASS "file-max is set 0 and doesn't crash"
> +}
> +
> +cleanup()
> +{
> +	sysctl -w "fs.""$name"=${orig_value} >/dev/null 2>&1
> +}
> +
> +tst_run
> -- 
> 2.18.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

On upstream kernel, before commit[1], the max value in proc_get_long based on
the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
but accepts values such as 18446744073709551616 (20 chars). But we should reject all
because both are overflows. After this commit,the permitted max value is 2^64-1.

Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
commit sets the max and min value for file-max. After this commit,the permitted max
value is 2^63-1.

Unfortunately, commit[2] introduced the minimum value points at the global 'zero'
variable which is an int. This results in a KASAN splat when accessed as a long by
proc_doulongvec_minmax on 64-bit architectures. This bug has been fixed by commit[3].

I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
[3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/commands                      |  1 +
 testcases/commands/sysctl/sysctl02.sh | 96 +++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100755 testcases/commands/sysctl/sysctl02.sh

diff --git a/runtest/commands b/runtest/commands
index ac15e8b23..1870c4209 100644
--- a/runtest/commands
+++ b/runtest/commands
@@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
 gdb01_sh gdb01.sh
 unshare01_sh unshare01.sh
 sysctl01_sh sysctl01.sh
+sysctl02_sh sysctl02.sh
diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
new file mode 100755
index 000000000..22dd0a429
--- /dev/null
+++ b/testcases/commands/sysctl/sysctl02.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2019 FUJITSU LIMITED. All rights reserved.
+# Author: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
+#
+# Description:
+# This is a regression test for handling overflow for file-max.
+#
+# when writing 2^64 to /proc/sys/fs/file-max. It will overflow
+# and be set to 0. It crash system quickly.
+#
+# The kernel bug has been fixed in kernel:
+# '7f2923c4f' (sysctl: handle overflow in proc_get_long)
+# the permitted max value is  2^64-1.
+# '32a5ad9c2' (sysctl: handle overflow for file-max)
+# the permitted max value is 2^63-1
+#
+# After merged this patchset, if we exceed the max value, it will
+# keep old value. Unfortunately, it introudced a new bug when set it
+# to 0 and it will lead to system crash.
+# This bug has been fixed by commit 9002b2146
+# (kernel/sysctl.c: fix out-of-bounds access when setting file-max)
+
+TST_TESTFUNC=do_test
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+TST_CNT=4
+TST_NEEDS_ROOT=1
+TST_NEEDS_CMDS="sysctl"
+dir="/proc/sys/fs/"
+syms_file="/proc/kallsyms"
+name="file-max"
+orig_value=200000
+
+. tst_test.sh
+
+setup()
+{
+	[ ! -f "$dir""$name" ] && tst_brk TCONF \
+		"$name was not supported"
+	orig_value=$(cat "$dir""$name")
+}
+
+do_test()
+{
+	case $1 in
+	1)sysctl_test_overflow 18446744073709551616;;
+	2)sysctl_test_overflow 18446744073709551615;;
+	3)sysctl_test_overflow 9223372036854775808;;
+	4)sysctl_test_zero;;
+	esac
+}
+
+sysctl_test_overflow()
+{
+	local old_value=$(cat "$dir""$name")
+
+	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
+
+	local test_value=$(cat "$dir""$name")
+
+	echo ${test_value} |grep -q ${old_value}
+	if [ $? -eq 0 ]; then
+		tst_res TPASS "file-max overflow, reject it and keep old value."
+	else
+		tst_res TFAIL "file-max overflow and set it to ${test_value}."
+	fi
+	cleanup
+}
+
+sysctl_test_zero()
+{
+	sysctl -w "fs.file-max"=0 >/dev/null 2>&1
+	[ ! -f "$syms_file" ] && tst_brk TCONF \
+		"$syms_file was not supported"
+	cat $syms_file  |grep kasan_report >/dev/null 2>&1
+	if [ $? -eq 0 ]; then
+		dmesg | grep "KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax" >/dev/null
+		if [ $? -eq 0 ]; then
+			tst_res TFAIL "file-max is set 0 and trigger a KASAN error"
+		else
+			tst_res TPASS \
+				"file-max is set 0 and doesn't trigger a KASAN error"
+		fi
+	else
+		tst_res TCONF "kernel doesn't support KASAN"
+	fi
+}
+
+cleanup()
+{
+	sysctl -w "fs.""$name"=${orig_value} >/dev/null 2>&1
+}
+
+tst_run
-- 
2.18.1

[LTP] [PATCH v2] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

Hi cryil

> Hi!
> +{
> +	case $1 in
> +	1)sysctl_test_overflow ${check1};;
> +	2)sysctl_test_overflow ${check2};;
> +	3)sysctl_test_overflow ${check3};;
> +	4)sysctl_test_zero ${check4};;
> There is no point in having the numbers in check variables if we do case
> here, we can just pass it here.
OK. I will pass the number directly.

>> +	esac
>> +}
>> +
>> +sysctl_test_overflow()
>> +{
>> +	local old_value=$(cat "$dir""$name")
>> +
>> +	sysctl -w "fs.file-max"=$1>/dev/null 2>&1
>> +
>> +	local test_value=$(cat "$dir""$name")
>> +
>> +	echo ${test_value} |grep -q ${old_value}
>> +	if [ $? -eq 0 ]; then
>> +		tst_res TPASS "file-max overflow, reject it and keep old value."
>> +	else
>> +		tst_res TFAIL "file-max overflow and set it to ${test_value}."
>> +	fi
>> +	cleanup
>> +}
>> +
>> +sysctl_test_zero()
>> +{
>> +	sysctl -w "fs.file-max"=$1>/dev/null 2>&1
>> +	tst_res TINFO "if it doesn't report TPASS after 60s sleep, system crashes"
>> +	sleep 60
> What happens on the buggy kernel here?
>
> Does it crash reliably?
>
> It looks to me that reproducing this bug without KASAN enabled kernel
> would be really hard or even impossible, therefore the sleep here does
> not matter at all.
Yes.  It crashes always on my machine but not having crash log (under /var/crash) without CONFIF_KASAN, and it hangs when sleep 120s with CONFIG_KASAN=y.
I think it crashes or hangs because the file-max is equal to 0 and it causes some system services or daemons can not run normally. Test it makes no sense.

This is not what I want to test.  I only want to test whether trigger a KASAN error by setting 0.
I will send a v3 patch.

>> +	tst_res TPASS "file-max is set 0 and doesn't crash"
>> +}
>> +
>> +cleanup()
>> +{
>> +	sysctl -w "fs.""$name"=${orig_value}>/dev/null 2>&1
>> +}
>> +
>> +tst_run
>> -- 
>> 2.18.1
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Petr Vorel]

Hi,

LGTM, below are just small formatting issues.
I'm going to merge it with these changes below.

> On upstream kernel, before commit[1], the max value in proc_get_long based on
> the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
> but accepts values such as 18446744073709551616 (20 chars). But we should reject all
> because both are overflows. After this commit,the permitted max value is 2^64-1.

> Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
> /proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
> commit sets the max and min value for file-max. After this commit,the permitted max
> value is 2^63-1.

> Unfortunately, commit[2] introduced the minimum value points at the global 'zero'
> variable which is an int. This results in a KASAN splat when accessed as a long by
> proc_doulongvec_minmax on 64-bit architectures. This bug has been fixed by commit[3].

> I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.

> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
> [3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972

> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  runtest/commands                      |  1 +
>  testcases/commands/sysctl/sysctl02.sh | 96 +++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>  create mode 100755 testcases/commands/sysctl/sysctl02.sh

> diff --git a/runtest/commands b/runtest/commands
> index ac15e8b23..1870c4209 100644
> --- a/runtest/commands
> +++ b/runtest/commands
> @@ -40,3 +40,4 @@ keyctl01_sh keyctl01.sh
>  gdb01_sh gdb01.sh
>  unshare01_sh unshare01.sh
>  sysctl01_sh sysctl01.sh
> +sysctl02_sh sysctl02.sh
> diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
> new file mode 100755
> index 000000000..22dd0a429
> --- /dev/null
> +++ b/testcases/commands/sysctl/sysctl02.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2019 FUJITSU LIMITED. All rights reserved.
> +# Author: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
> +#
> +# Description:
> +# This is a regression test for handling overflow for file-max.
Instead these 2 lines I'd put just this (nit):
# Regression test for handling overflow for file-max.

> +#
> +# when writing 2^64 to /proc/sys/fs/file-max. It will overflow
> +# and be set to 0. It crash system quickly.
> +#
> +# The kernel bug has been fixed in kernel:
> +# '7f2923c4f' (sysctl: handle overflow in proc_get_long)
> +# the permitted max value is  2^64-1.
> +# '32a5ad9c2' (sysctl: handle overflow for file-max)
> +# the permitted max value is 2^63-1
> +#
> +# After merged this patchset, if we exceed the max value, it will
> +# keep old value. Unfortunately, it introudced a new bug when set it
> +# to 0 and it will lead to system crash.
> +# This bug has been fixed by commit 9002b2146
> +# (kernel/sysctl.c: fix out-of-bounds access when setting file-max)
> +
> +TST_TESTFUNC=do_test
> +TST_SETUP=setup
> +TST_CLEANUP=cleanup
> +TST_CNT=4
> +TST_NEEDS_ROOT=1
> +TST_NEEDS_CMDS="sysctl"
> +dir="/proc/sys/fs/"
> +syms_file="/proc/kallsyms"
> +name="file-max"
> +orig_value=200000
> +
> +. tst_test.sh
> +
> +setup()
> +{
> +	[ ! -f "$dir""$name" ] && tst_brk TCONF \
> +		"$name was not supported"
[ ! -f "$dir$name" ] && tst_brk TCONF "$name was not supported"

> +	orig_value=$(cat "$dir""$name")
> +}
> +
> +do_test()
> +{
> +	case $1 in
> +	1)sysctl_test_overflow 18446744073709551616;;
> +	2)sysctl_test_overflow 18446744073709551615;;
> +	3)sysctl_test_overflow 9223372036854775808;;
> +	4)sysctl_test_zero;;
> +	esac
> +}
> +
> +sysctl_test_overflow()
> +{
> +	local old_value=$(cat "$dir""$name")
> +
> +	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
> +
> +	local test_value=$(cat "$dir""$name")
again, why 2 stings instead of one?
local test_value=$(cat "$dir$name")

> +
> +	echo ${test_value} |grep -q ${old_value}
> +	if [ $? -eq 0 ]; then
> +		tst_res TPASS "file-max overflow, reject it and keep old value."
Please don't use dot

> +	else
> +		tst_res TFAIL "file-max overflow and set it to ${test_value}."
Here as well.

> +	fi
> +	cleanup
> +}
> +
> +sysctl_test_zero()
> +{
> +	sysctl -w "fs.file-max"=0 >/dev/null 2>&1
Please use -q option instead of redirecting.
> +	[ ! -f "$syms_file" ] && tst_brk TCONF \
> +		"$syms_file was not supported"

> +	cat $syms_file  |grep kasan_report >/dev/null 2>&1
> +	if [ $? -eq 0 ]; then
Also grep has -q, no need to use pipe:

if grep -q kasan_report $syms_file; then

> +		dmesg | grep "KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax" >/dev/null
-q here as well.

> +		if [ $? -eq 0 ]; then
> +			tst_res TFAIL "file-max is set 0 and trigger a KASAN error"
> +		else
> +			tst_res TPASS \
> +				"file-max is set 0 and doesn't trigger a KASAN error"
Probably can be on single line.

> +		fi
> +	else
> +		tst_res TCONF "kernel doesn't support KASAN"
> +	fi
> +}
> +
> +cleanup()
> +{
> +	sysctl -w "fs.""$name"=${orig_value} >/dev/null 2>&1
It's safe not quote string ($name is safe, but you can of course)
sysctl -q -w fs.$name=$orig_value

> +}
> +
> +tst_run

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Petr Vorel]

Hi Yang,

> On upstream kernel, before commit[1], the max value in proc_get_long based on
> the number of chars(21). It rejects values such as 184467440737095516160 (21 chars)
> but accepts values such as 18446744073709551616 (20 chars). But we should reject all
> because both are overflows. After this commit,the permitted max value is 2^64-1.

> Before commit[2], when writing echo 18446744073709551616 > /proc/sys/fs/file-max
> /proc/sys/fs/file-max will overflow and be set to 0.  It may crash the system. This
> commit sets the max and min value for file-max. After this commit,the permitted max
> value is 2^63-1.

> Unfortunately, commit[2] introduced the minimum value points at the global 'zero'
> variable which is an int. This results in a KASAN splat when accessed as a long by
> proc_doulongvec_minmax on 64-bit architectures. This bug has been fixed by commit[3].

> I will set 2^64 ,2^64-1,2^63 and 0 to file-max in case and test it.

> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f2923c4f73f21cfd714d12a2d48de8c21f11cfe
> [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=32a5ad9c22852e6bd9e74bdec5934ef9d1480bc5
> [3]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9002b21465fa4d829edfc94a5a441005cffaa972


Merged with adjusted commit message and following diff.
Due que big changes I put my copyright there as well.

Thanks for your patch!

Kind regards,
Petr

diff --git testcases/commands/sysctl/sysctl02.sh testcases/commands/sysctl/sysctl02.sh
index 22dd0a429..8a434183e 100755
--- testcases/commands/sysctl/sysctl02.sh
+++ testcases/commands/sysctl/sysctl02.sh
@@ -1,26 +1,18 @@
 #!/bin/sh
-
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2019 FUJITSU LIMITED. All rights reserved.
+# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
 # Author: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
 #
-# Description:
-# This is a regression test for handling overflow for file-max.
-#
-# when writing 2^64 to /proc/sys/fs/file-max. It will overflow
-# and be set to 0. It crash system quickly.
+# Test for these regressions causing buffer overflow when writing into
+# /proc/sys/fs/file-max:
+# 7f2923c4f73f ("sysctl: handle overflow in proc_get_long")
+# 32a5ad9c2285 ("sysctl: handle overflow for file-max")
 #
-# The kernel bug has been fixed in kernel:
-# '7f2923c4f' (sysctl: handle overflow in proc_get_long)
-# the permitted max value is  2^64-1.
-# '32a5ad9c2' (sysctl: handle overflow for file-max)
-# the permitted max value is 2^63-1
+# This bug has been fixed in 9002b21465fa ("kernel/sysctl.c: fix
+# out-of-bounds access when setting file-max")
 #
-# After merged this patchset, if we exceed the max value, it will
-# keep old value. Unfortunately, it introudced a new bug when set it
-# to 0 and it will lead to system crash.
-# This bug has been fixed by commit 9002b2146
-# (kernel/sysctl.c: fix out-of-bounds access when setting file-max)
+# We test in sysctl02.sh setting 2^64, 2^64-1, 2^63 and 0.
 
 TST_TESTFUNC=do_test
 TST_SETUP=setup
@@ -28,60 +20,52 @@ TST_CLEANUP=cleanup
 TST_CNT=4
 TST_NEEDS_ROOT=1
 TST_NEEDS_CMDS="sysctl"
-dir="/proc/sys/fs/"
+sys_name="fs.file-max"
+sys_file="/proc/sys/fs/file-max"
 syms_file="/proc/kallsyms"
-name="file-max"
-orig_value=200000
 
 . tst_test.sh
 
 setup()
 {
-	[ ! -f "$dir""$name" ] && tst_brk TCONF \
-		"$name was not supported"
-	orig_value=$(cat "$dir""$name")
+	[ ! -f "$sys_file" ] && tst_brk TCONF "$sys_file not enabled"
+	orig_value=$(cat "$sys_file")
 }
 
 do_test()
 {
 	case $1 in
-	1)sysctl_test_overflow 18446744073709551616;;
-	2)sysctl_test_overflow 18446744073709551615;;
-	3)sysctl_test_overflow 9223372036854775808;;
-	4)sysctl_test_zero;;
+	1) sysctl_test_overflow 18446744073709551616 ;;
+	2) sysctl_test_overflow 18446744073709551615 ;;
+	3) sysctl_test_overflow 9223372036854775808 ;;
+	4) sysctl_test_zero ;;
 	esac
 }
 
 sysctl_test_overflow()
 {
-	local old_value=$(cat "$dir""$name")
-
-	sysctl -w "fs.file-max"=$1 >/dev/null 2>&1
+	local old_value="$(cat $sys_file)"
+	sysctl -w -q $sys_name=$1 2>/dev/null
+	local test_value="$(cat $sys_file)"
 
-	local test_value=$(cat "$dir""$name")
-
-	echo ${test_value} |grep -q ${old_value}
-	if [ $? -eq 0 ]; then
-		tst_res TPASS "file-max overflow, reject it and keep old value."
+	if echo $test_value | grep -q $old_value; then
+		tst_res TPASS "$sys_file overflows, reject it and keep old value"
 	else
-		tst_res TFAIL "file-max overflow and set it to ${test_value}."
+		tst_res TFAIL "$sys_file overflows and set to $test_value"
 	fi
 	cleanup
 }
 
 sysctl_test_zero()
 {
-	sysctl -w "fs.file-max"=0 >/dev/null 2>&1
-	[ ! -f "$syms_file" ] && tst_brk TCONF \
-		"$syms_file was not supported"
-	cat $syms_file  |grep kasan_report >/dev/null 2>&1
-	if [ $? -eq 0 ]; then
-		dmesg | grep "KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax" >/dev/null
-		if [ $? -eq 0 ]; then
-			tst_res TFAIL "file-max is set 0 and trigger a KASAN error"
+	[ ! -f "$syms_file" ] && tst_brk TCONF "$syms_file not enabled"
+	ROD sysctl -w -q $sys_name=0
+
+	if grep -q kasan_report $syms_file; then
+		if dmesg | grep -q "KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax"; then
+			tst_res TFAIL "$sys_file is set 0 and trigger a KASAN error"
 		else
-			tst_res TPASS \
-				"file-max is set 0 and doesn't trigger a KASAN error"
+			tst_res TPASS "$sys_file is set 0 and doesn't trigger a KASAN error"
 		fi
 	else
 		tst_res TCONF "kernel doesn't support KASAN"
@@ -90,7 +74,7 @@ sysctl_test_zero()
 
 cleanup()
 {
-	sysctl -w "fs.""$name"=${orig_value} >/dev/null 2>&1
+	[ -n "$orig_value" ] && sysctl -w -q $sys_name=$orig_value
 }
 
 tst_run

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

> Hi,
>
> LGTM, below are just small formatting issues.
> I'm going to merge it with these changes below.
>
>> Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
>> ---
>>   runtest/commands                      |  1 +
>>   testcases/commands/sysctl/sysctl02.sh | 96 +++++++++++++++++++++++++++
>>   2 files changed, 97 insertions(+)
>>   create mode 100755 testcases/commands/sysctl/sysctl02.sh
> ...
>> +sysctl_test_overflow()
>> +{
>> +	local old_value=$(cat "$dir""$name")
>> +
>> +	sysctl -w "fs.file-max"=$1>/dev/null 2>&1
>> +
>> +	local test_value=$(cat "$dir""$name")
> again, why 2 stings instead of one?
> local test_value=$(cat "$dir$name")
Hi Petr

  I use 2 strings because sysctl uses "fs.file-max" but cat uses "fs/file-max" file.
  Your big changes look good to me.  Thanks for your changes.

>> +
>> +	echo ${test_value} |grep -q ${old_value}
>> +	if [ $? -eq 0 ]; then
>> +		tst_res TPASS "file-max overflow, reject it and keep old value."
> Please don't use dot
>
>> +	else
>> +		tst_res TFAIL "file-max overflow and set it to ${test_value}."
> Here as well.
>
>> +	fi
>> +	cleanup
>> +}
>> +
>> +sysctl_test_zero()
>> +{
>> +	sysctl -w "fs.file-max"=0>/dev/null 2>&1
> Please use -q option instead of redirecting.
>> +	[ ! -f "$syms_file" ]&&  tst_brk TCONF \
>> +		"$syms_file was not supported"
>> +	cat $syms_file  |grep kasan_report>/dev/null 2>&1
>> +	if [ $? -eq 0 ]; then
> Also grep has -q, no need to use pipe:
>
> if grep -q kasan_report $syms_file; then
>
>> +		dmesg | grep "KASAN: global-out-of-bounds in __do_proc_doulongvec_minmax">/dev/null
> -q here as well.
>
>> +		if [ $? -eq 0 ]; then
>> +			tst_res TFAIL "file-max is set 0 and trigger a KASAN error"
>> +		else
>> +			tst_res TPASS \
>> +				"file-max is set 0 and doesn't trigger a KASAN error"
> Probably can be on single line.
>
>> +		fi
>> +	else
>> +		tst_res TCONF "kernel doesn't support KASAN"
>> +	fi
>> +}
>> +
>> +cleanup()
>> +{
>> +	sysctl -w "fs.""$name"=${orig_value}>/dev/null 2>&1
> It's safe not quote string ($name is safe, but you can of course)
> sysctl -q -w fs.$name=$orig_value
>
>> +}
>> +
>> +tst_run
>
> .
>

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Petr Vorel]

Hi Yang,

> Hi Petr

>  I use 2 strings because sysctl uses "fs.file-max" but cat uses "fs/file-max" file.
>  Your big changes look good to me.  Thanks for your changes.
I pushed tiny changes to sysctl02.sh as 8e2d73a88 ("sysctl/sysctl02: Improve log messages").
See the diff below.

Kind regards,
Petr

diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
index 8a434183e..3964a9829 100755
--- a/testcases/commands/sysctl/sysctl02.sh
+++ b/testcases/commands/sysctl/sysctl02.sh
@@ -44,14 +44,17 @@ do_test()
 
 sysctl_test_overflow()
 {
+	local test_value="$1"
 	local old_value="$(cat $sys_file)"
-	sysctl -w -q $sys_name=$1 2>/dev/null
-	local test_value="$(cat $sys_file)"
 
-	if echo $test_value | grep -q $old_value; then
-		tst_res TPASS "$sys_file overflows, reject it and keep old value"
+	tst_res TINFO "trying to set $sys_name=$test_value"
+	sysctl -w -q $sys_name=$test_value 2>/dev/null
+	local new_value="$(cat $sys_file)"
+
+	if [ "$new_value" = "$old_value" ]; then
+		tst_res TPASS "$sys_file keeps old value ($old_value)"
 	else
-		tst_res TFAIL "$sys_file overflows and set to $test_value"
+		tst_res TFAIL "$sys_file overflows and is set to $new_value"
 	fi
 	cleanup
 }

[LTP] [PATCH v3] sysctl/sysctl02: Add new regression test for overflow file-max

[Yang Xu]

> Hi Yang,
>
>> Hi Petr
>>   I use 2 strings because sysctl uses "fs.file-max" but cat uses "fs/file-max" file.
>>   Your big changes look good to me.  Thanks for your changes.
> I pushed tiny changes to sysctl02.sh as 8e2d73a88 ("sysctl/sysctl02: Improve log messages").
> See the diff below.
Hi Petr

     Your change makes this case more clear for user. It looks good to me.

Thanks
Yang Xu

> Kind regards,
> Petr
>
> diff --git a/testcases/commands/sysctl/sysctl02.sh b/testcases/commands/sysctl/sysctl02.sh
> index 8a434183e..3964a9829 100755
> --- a/testcases/commands/sysctl/sysctl02.sh
> +++ b/testcases/commands/sysctl/sysctl02.sh
> @@ -44,14 +44,17 @@ do_test()
>
>   sysctl_test_overflow()
>   {
> +	local test_value="$1"
>   	local old_value="$(cat $sys_file)"
> -	sysctl -w -q $sys_name=$1 2>/dev/null
> -	local test_value="$(cat $sys_file)"
>
> -	if echo $test_value | grep -q $old_value; then
> -		tst_res TPASS "$sys_file overflows, reject it and keep old value"
> +	tst_res TINFO "trying to set $sys_name=$test_value"
> +	sysctl -w -q $sys_name=$test_value 2>/dev/null
> +	local new_value="$(cat $sys_file)"
> +
> +	if [ "$new_value" = "$old_value" ]; then
> +		tst_res TPASS "$sys_file keeps old value ($old_value)"
>   	else
> -		tst_res TFAIL "$sys_file overflows and set to $test_value"
> +		tst_res TFAIL "$sys_file overflows and is set to $new_value"
>   	fi
>   	cleanup
>   }
>
>
> .
>