[PATCH v4 0/2] Fixes multiple sysctl proc_handler usage error

[PATCH v4 0/2] Fixes multiple sysctl proc_handler usage error

[nicolas.bouchinet]

From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Hi, while reading sysctl code I encountered two sysctl proc_handler
parameters common errors.

The first one is to declare .data as a different type thant the return of
the used .proc_handler, i.e. using proch_dointvec, thats convert a char
string to signed integers, and storing the result in a .data that is backed
by an unsigned int. User can then write "-1" string, which results in a
different value stored in the .data variable. This can lead to type
conversion errors in branches and thus to potential security issues.

From a quick search using regex and only for proc_dointvec, this seems to
be a pretty common mistake.

The second one is to declare .extra1 or .extra2 values with a .proc_handler
that don't uses them. i.e, declaring .extra1 or .extra2 using proc_dointvec
in order to declare conversion bounds do not work as do_proc_dointvec don't
uses those variables if not explicitly asked.

This patchset corrects three sysctl declaration that are buggy as an
example and is not exhaustive.

Nicolas

---

Changes since v3:
https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@clip-os.org/

* Fixed patch 2/2 extra* parameter typo detected by Joel Granados.
* Reworded patch 2/2 as suggested by Joel Granados.


Changes since v2:
https://lore.kernel.org/all/20241114162638.57392-1-nicolas.bouchinet@clip-os.org/

* Bound vdso_enabled to 0 and 1 as suggested by Joel Granados.
* Remove patch 3/3 since Greg Kroah-Hartman merged it.

Changes since v1:
https://lore.kernel.org/all/20241112131357.49582-1-nicolas.bouchinet@clip-os.org/

* Take Lin Feng review into account.

---

Nicolas Bouchinet (2):
  coredump: Fixes core_pipe_limit sysctl proc_handler
  sysctl: Fix underflow value setting risk in vm_table

 arch/sh/kernel/vsyscall/vsyscall.c | 3 ++-
 fs/coredump.c                      | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.48.1

[PATCH v4 1/2] coredump: Fixes core_pipe_limit sysctl proc_handler

[nicolas.bouchinet]

From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

proc_dointvec converts a string to a vector of signed int, which is
stored in the unsigned int .data core_pipe_limit.
It was thus authorized to write a negative value to core_pipe_limit
sysctl which once stored in core_pipe_limit, leads to the signed int
dump_count check against core_pipe_limit never be true. The same can be
achieved with core_pipe_limit set to INT_MAX.

Any negative write or >= to INT_MAX in core_pipe_limit sysctl would
hypothetically allow a user to create very high load on the system by
running processes that produces a coredump in case the core_pattern
sysctl is configured to pipe core files to user space helper.
Memory or PID exhaustion should happen before but it anyway breaks the
core_pipe_limit semantic.

This commit fixes this by changing core_pipe_limit sysctl's proc_handler
to proc_dointvec_minmax and bound checking between SYSCTL_ZERO and
SYSCTL_INT_MAX.

Fixes: a293980c2e26 ("exec: let do_coredump() limit the number of concurrent dumps to pipes")
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/coredump.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index d48edb37bc35c..9239636b8812a 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -1015,7 +1015,9 @@ static struct ctl_table coredump_sysctls[] = {
 		.data		= &core_pipe_limit,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname       = "core_file_note_size_limit",
-- 
2.48.1

Re: [PATCH v4 1/2] coredump: Fixes core_pipe_limit sysctl proc_handler

[Kees Cook]

On Wed, Jan 15, 2025 at 02:22:08PM +0100, nicolas.bouchinet@clip-os.org wrote:
> Any negative write or >= to INT_MAX in core_pipe_limit sysctl would
> hypothetically allow a user to create very high load on the system by
> running processes that produces a coredump in case the core_pattern
> sysctl is configured to pipe core files to user space helper.
> Memory or PID exhaustion should happen before but it anyway breaks the
> core_pipe_limit semantic.

Isn't this true for "0" too (the default)? I'm not opposed to the change
since it makes things more clear, but I don't think the >=INT_MAX
problem is anything more than "functionally identical to 0". :)

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH v4 1/2] coredump: Fixes core_pipe_limit sysctl proc_handler

[Nicolas Bouchinet]

On 1/16/25 1:32 AM, Kees Cook wrote:
> On Wed, Jan 15, 2025 at 02:22:08PM +0100, nicolas.bouchinet@clip-os.org wrote:
>> Any negative write or >= to INT_MAX in core_pipe_limit sysctl would
>> hypothetically allow a user to create very high load on the system by
>> running processes that produces a coredump in case the core_pattern
>> sysctl is configured to pipe core files to user space helper.
>> Memory or PID exhaustion should happen before but it anyway breaks the
>> core_pipe_limit semantic.
> Isn't this true for "0" too (the default)? I'm not opposed to the change
> since it makes things more clear, but I don't think the >=INT_MAX
> problem is anything more than "functionally identical to 0". :)
Uhm, I think your right, its seems to be functionally identical.
0 codepath slightly differs from > 0 though since it won't trigger
wait_for_dump_helpers().

Thanks for your review,

Nicolas
>
> Reviewed-by: Kees Cook <kees@kernel.org>
>

[PATCH v4 2/2] sysctl: Fix underflow value setting risk in vm_table

[nicolas.bouchinet]

From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Commit 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
fixes underflow value setting risk in vm_table but misses vdso_enabled
sysctl.

vdso_enabled sysctl is initialized with .extra1 value as SYSCTL_ZERO to
avoid negative value writes but the proc_handler is proc_dointvec and
not proc_dointvec_minmax and thus do not uses .extra1 and .extra2.

The following command thus works :

`# echo -1 > /proc/sys/vm/vdso_enabled`

This patch properly sets the proc_handler to proc_dointvec_minmax.
In addition to .extra1, .extra2 is set to SYSCTL_ONE. The sysctl is
thus bounded between 0 and 1.

Fixes: 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 arch/sh/kernel/vsyscall/vsyscall.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/sh/kernel/vsyscall/vsyscall.c b/arch/sh/kernel/vsyscall/vsyscall.c
index d80dd92a483af..1563dcc55fd32 100644
--- a/arch/sh/kernel/vsyscall/vsyscall.c
+++ b/arch/sh/kernel/vsyscall/vsyscall.c
@@ -37,8 +37,9 @@ static const struct ctl_table vdso_table[] = {
 		.data		= &vdso_enabled,
 		.maxlen		= sizeof(vdso_enabled),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
 	},
 };
 
-- 
2.48.1

Re: [PATCH v4 2/2] sysctl: Fix underflow value setting risk in vm_table

[Kees Cook]

On Wed, Jan 15, 2025 at 02:22:09PM +0100, nicolas.bouchinet@clip-os.org wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Commit 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
> fixes underflow value setting risk in vm_table but misses vdso_enabled
> sysctl.
> 
> vdso_enabled sysctl is initialized with .extra1 value as SYSCTL_ZERO to
> avoid negative value writes but the proc_handler is proc_dointvec and
> not proc_dointvec_minmax and thus do not uses .extra1 and .extra2.
> 
> The following command thus works :
> 
> `# echo -1 > /proc/sys/vm/vdso_enabled`
> 
> This patch properly sets the proc_handler to proc_dointvec_minmax.
> In addition to .extra1, .extra2 is set to SYSCTL_ONE. The sysctl is
> thus bounded between 0 and 1.
> 
> Fixes: 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH v4 0/2] Fixes multiple sysctl proc_handler usage error

[Joel Granados]

On Wed, Jan 15, 2025 at 02:22:07PM +0100, nicolas.bouchinet@clip-os.org wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Hi, while reading sysctl code I encountered two sysctl proc_handler
> parameters common errors.
> 
> The first one is to declare .data as a different type thant the return of
> the used .proc_handler, i.e. using proch_dointvec, thats convert a char
> string to signed integers, and storing the result in a .data that is backed
> by an unsigned int. User can then write "-1" string, which results in a
> different value stored in the .data variable. This can lead to type
> conversion errors in branches and thus to potential security issues.
> 
> From a quick search using regex and only for proc_dointvec, this seems to
> be a pretty common mistake.
> 
> The second one is to declare .extra1 or .extra2 values with a .proc_handler
> that don't uses them. i.e, declaring .extra1 or .extra2 using proc_dointvec
> in order to declare conversion bounds do not work as do_proc_dointvec don't
> uses those variables if not explicitly asked.
> 
> This patchset corrects three sysctl declaration that are buggy as an
> example and is not exhaustive.
After some time in sysctl-testing, pushing this to sysctl-next

> 
> Nicolas
> 
> ---
> 
> Changes since v3:
> https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@clip-os.org/
> 
> * Fixed patch 2/2 extra* parameter typo detected by Joel Granados.
> * Reworded patch 2/2 as suggested by Joel Granados.
> 
> 
> Changes since v2:
> https://lore.kernel.org/all/20241114162638.57392-1-nicolas.bouchinet@clip-os.org/
> 
> * Bound vdso_enabled to 0 and 1 as suggested by Joel Granados.
> * Remove patch 3/3 since Greg Kroah-Hartman merged it.
> 
> Changes since v1:
> https://lore.kernel.org/all/20241112131357.49582-1-nicolas.bouchinet@clip-os.org/
> 
> * Take Lin Feng review into account.
> 
> ---
> 
> Nicolas Bouchinet (2):
>   coredump: Fixes core_pipe_limit sysctl proc_handler
>   sysctl: Fix underflow value setting risk in vm_table
> 
>  arch/sh/kernel/vsyscall/vsyscall.c | 3 ++-
>  fs/coredump.c                      | 4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> -- 
> 2.48.1
> 

-- 

Joel Granados