[PATCH 0/3] Fixes multiple sysctl proc_handler usage error

[PATCH 0/3] 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

Nicolas Bouchinet (3):
  coredump: Fixes core_pipe_limit sysctl proc_handler
  sysctl: Fix underflow value setting risk in vm_table
  tty: ldsic: fix tty_ldisc_autoload sysctl's proc_handler

 drivers/tty/tty_io.c | 2 +-
 fs/coredump.c        | 7 +++++--
 kernel/sysctl.c      | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.47.0

[PATCH 1/3] 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>
---
 fs/coredump.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 7f12ff6ad1d3e..8ea5896e518dd 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -616,7 +616,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		cprm.limit = RLIM_INFINITY;
 
 		dump_count = atomic_inc_return(&core_dump_count);
-		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
+		if ((core_pipe_limit && (core_pipe_limit < dump_count)) ||
+		    (core_pipe_limit && dump_count == INT_MAX)) {
 			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
 			       task_tgid_vnr(current), current->comm);
 			printk(KERN_WARNING "Skipping core dump\n");
@@ -1024,7 +1025,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.47.0

[PATCH 2/3] 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.

Fixes: 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
 kernel/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 79e6cb1d5c48f..37b1c1a760985 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2194,7 +2194,7 @@ static struct ctl_table vm_table[] = {
 		.maxlen		= sizeof(vdso_enabled),
 #endif
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
 	},
 #endif
-- 
2.47.0

[PATCH 3/3] tty: ldsic: fix tty_ldisc_autoload sysctl's proc_handler

[nicolas.bouchinet]

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

Commit 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of
ldiscs") introduces the tty_ldisc_autoload sysctl with the wrong
proc_handler. .extra1 and .extra2 parameters are set to avoid other values
thant SYSCTL_ZERO or SYSCTL_ONE to be set but proc_dointvec do not uses
them.

This commit fixes this by using proc_dointvec_minmax instead of
proc_dointvec.

Fixes: 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of ldiscs")
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c10..f211154367420 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3631,7 +3631,7 @@ static struct ctl_table tty_table[] = {
 		.data		= &tty_ldisc_autoload,
 		.maxlen		= sizeof(tty_ldisc_autoload),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},
-- 
2.47.0

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

[Lin Feng]

Hi,

see comments below please.

On 11/12/24 21:13, nicolas.bouchinet@clip-os.org wrote:
> 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>
> ---
>  fs/coredump.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 7f12ff6ad1d3e..8ea5896e518dd 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -616,7 +616,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  		cprm.limit = RLIM_INFINITY;
>  
>  		dump_count = atomic_inc_return(&core_dump_count);
> -		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
> +		if ((core_pipe_limit && (core_pipe_limit < dump_count)) ||
> +		    (core_pipe_limit && dump_count == INT_MAX)) {

While comparing between 'unsigned int' and 'signed int', C deems them both
to 'unsigned int', so as an insane user sets core_pipe_limit to INT_MAX,
and dump_count(signed int) does overflow INT_MAX, checking for 
'core_pipe_limit < dump_count' is passed, thus codes skips core dump.

So IMO it's enough after changing proc_handler to proc_dointvec_minmax.

Others in this patch:
Reviewed-by: Lin Feng <linf@wangsu.com>

>  			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
>  			       task_tgid_vnr(current), current->comm);
>  			printk(KERN_WARNING "Skipping core dump\n");
> @@ -1024,7 +1025,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",

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

[Lin Feng]

Thanks!

Reviewed-by: Lin Feng <linf@wangsu.com>

On 11/12/24 21:13, 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.
> 
> Fixes: 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table")
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48f..37b1c1a760985 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2194,7 +2194,7 @@ static struct ctl_table vm_table[] = {
>  		.maxlen		= sizeof(vdso_enabled),
>  #endif
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= SYSCTL_ZERO,
>  	},
>  #endif

Re: [PATCH 3/3] tty: ldsic: fix tty_ldisc_autoload sysctl's proc_handler

[Lin Feng]

Thanks!

Reviewed-by: Lin Feng <linf@wangsu.com>

On 11/12/24 21:13, nicolas.bouchinet@clip-os.org wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Commit 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of
> ldiscs") introduces the tty_ldisc_autoload sysctl with the wrong
> proc_handler. .extra1 and .extra2 parameters are set to avoid other values
> thant SYSCTL_ZERO or SYSCTL_ONE to be set but proc_dointvec do not uses
> them.
> 
> This commit fixes this by using proc_dointvec_minmax instead of
> proc_dointvec.
> 
> Fixes: 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of ldiscs")
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  drivers/tty/tty_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 407b0d87b7c10..f211154367420 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3631,7 +3631,7 @@ static struct ctl_table tty_table[] = {
>  		.data		= &tty_ldisc_autoload,
>  		.maxlen		= sizeof(tty_ldisc_autoload),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= SYSCTL_ZERO,
>  		.extra2		= SYSCTL_ONE,
>  	},

Re: [PATCH 3/3] tty: ldsic: fix tty_ldisc_autoload sysctl's proc_handler

[Jiri Slaby]

On 12. 11. 24, 14:13, nicolas.bouchinet@clip-os.org wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Commit 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of
> ldiscs") introduces the tty_ldisc_autoload sysctl with the wrong
> proc_handler. .extra1 and .extra2 parameters are set to avoid other values
> thant SYSCTL_ZERO or SYSCTL_ONE to be set but proc_dointvec do not uses
> them.
> 
> This commit fixes this by using proc_dointvec_minmax instead of
> proc_dointvec.
> 
> Fixes: 7c0cca7c847e ("tty: ldisc: add sysctl to prevent autoloading of ldiscs")
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

I hope noone managed to store 2+ to that sysctl yet...

thanks,
-- 
js
suse labs

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

[Nicolas Bouchinet]

Hi Lin,

Thanks for your review.

On 11/13/24 03:35, Lin Feng wrote:
> Hi,
>
> see comments below please.
>
> On 11/12/24 21:13, nicolas.bouchinet@clip-os.org wrote:
>> 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>
>> ---
>>   fs/coredump.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/coredump.c b/fs/coredump.c
>> index 7f12ff6ad1d3e..8ea5896e518dd 100644
>> --- a/fs/coredump.c
>> +++ b/fs/coredump.c
>> @@ -616,7 +616,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>>   		cprm.limit = RLIM_INFINITY;
>>   
>>   		dump_count = atomic_inc_return(&core_dump_count);
>> -		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
>> +		if ((core_pipe_limit && (core_pipe_limit < dump_count)) ||
>> +		    (core_pipe_limit && dump_count == INT_MAX)) {
> While comparing between 'unsigned int' and 'signed int', C deems them both
> to 'unsigned int', so as an insane user sets core_pipe_limit to INT_MAX,
> and dump_count(signed int) does overflow INT_MAX, checking for
> 'core_pipe_limit < dump_count' is passed, thus codes skips core dump.
>
> So IMO it's enough after changing proc_handler to proc_dointvec_minmax.

Indeed, but the dump_count == INT_MAX is not here to catch overflow but 
if both dump_count
and core_pipe_limit are equal to INT_MAX. core_pipe_limit will not be 
inferior to dump_count.
Or maybe I am missing something ?

I should factorize the test though, this is kind of ugly.

>
> Others in this patch:
> Reviewed-by: Lin Feng <linf@wangsu.com>
>
>>   			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
>>   			       task_tgid_vnr(current), current->comm);
>>   			printk(KERN_WARNING "Skipping core dump\n");
>> @@ -1024,7 +1025,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",
>

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

[Lin Feng]

Hi, 

On 11/13/24 22:15, Nicolas Bouchinet wrote:
> Hi Lin,
> 
> Thanks for your review.
> 
> On 11/13/24 03:35, Lin Feng wrote:
>> Hi,
>>
>> see comments below please.
>>
>> On 11/12/24 21:13, nicolas.bouchinet@clip-os.org wrote:
>>> 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>
>>> ---
>>>   fs/coredump.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/coredump.c b/fs/coredump.c
>>> index 7f12ff6ad1d3e..8ea5896e518dd 100644
>>> --- a/fs/coredump.c
>>> +++ b/fs/coredump.c
>>> @@ -616,7 +616,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>>>   		cprm.limit = RLIM_INFINITY;
>>>   
>>>   		dump_count = atomic_inc_return(&core_dump_count);
>>> -		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
>>> +		if ((core_pipe_limit && (core_pipe_limit < dump_count)) ||
>>> +		    (core_pipe_limit && dump_count == INT_MAX)) {
>> While comparing between 'unsigned int' and 'signed int', C deems them both
>> to 'unsigned int', so as an insane user sets core_pipe_limit to INT_MAX,
>> and dump_count(signed int) does overflow INT_MAX, checking for
>> 'core_pipe_limit < dump_count' is passed, thus codes skips core dump.
>>
>> So IMO it's enough after changing proc_handler to proc_dointvec_minmax.
> Indeed, but the dump_count == INT_MAX is not here to catch overflow but 
> if both dump_count
> and core_pipe_limit are equal to INT_MAX. core_pipe_limit will not be 
> inferior to dump_count.
> Or maybe I am missing something ?
> 
Extracted from man core:
       Since Linux 2.6.32, the /proc/sys/kernel/core_pipe_limit can be used to
       defend against this possibility.  The value in this  file  defines  how
       many  concurrent crashing processes may be piped to user-space programs
       in parallel.  If this value is exceeded, then those crashing  processes
       above  this  value are noted in the kernel log and their core dumps are
       skipped.

Since no spinlock protecting us, due to the concurrent running of
atomic_inc_return(&core_dump_count), even with the changing above
it's not guaranteed that core_dump_count can't exceed core_pipe_limit).
As you said, suppose both of them are equal to INT_MAX(0x7fffffff),
and before any dummping thread drops core_dump_count, one new thread
comes in then hits atomic_inc_return(&core_dump_count) and now
(unsigned int)core_dump_count is 0x80000000, but original codes checking
for core_pipe_limit still works as expected.

Please correct me if I'm wrong :)

Thanks,
linfeng

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

[Nicolas Bouchinet]

On 11/14/24 02:34, Lin Feng wrote:
> Hi,
>
> On 11/13/24 22:15, Nicolas Bouchinet wrote:
>> Hi Lin,
>>
>> Thanks for your review.
>>
>> On 11/13/24 03:35, Lin Feng wrote:
>>> Hi,
>>>
>>> see comments below please.
>>>
>>> On 11/12/24 21:13, nicolas.bouchinet@clip-os.org wrote:
>>>> 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>
>>>> ---
>>>>    fs/coredump.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/coredump.c b/fs/coredump.c
>>>> index 7f12ff6ad1d3e..8ea5896e518dd 100644
>>>> --- a/fs/coredump.c
>>>> +++ b/fs/coredump.c
>>>> @@ -616,7 +616,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>>>>    		cprm.limit = RLIM_INFINITY;
>>>>    
>>>>    		dump_count = atomic_inc_return(&core_dump_count);
>>>> -		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
>>>> +		if ((core_pipe_limit && (core_pipe_limit < dump_count)) ||
>>>> +		    (core_pipe_limit && dump_count == INT_MAX)) {
>>> While comparing between 'unsigned int' and 'signed int', C deems them both
>>> to 'unsigned int', so as an insane user sets core_pipe_limit to INT_MAX,
>>> and dump_count(signed int) does overflow INT_MAX, checking for
>>> 'core_pipe_limit < dump_count' is passed, thus codes skips core dump.
>>>
>>> So IMO it's enough after changing proc_handler to proc_dointvec_minmax.
>> Indeed, but the dump_count == INT_MAX is not here to catch overflow but
>> if both dump_count
>> and core_pipe_limit are equal to INT_MAX. core_pipe_limit will not be
>> inferior to dump_count.
>> Or maybe I am missing something ?
>>
> Extracted from man core:
>         Since Linux 2.6.32, the /proc/sys/kernel/core_pipe_limit can be used to
>         defend against this possibility.  The value in this  file  defines  how
>         many  concurrent crashing processes may be piped to user-space programs
>         in parallel.  If this value is exceeded, then those crashing  processes
>         above  this  value are noted in the kernel log and their core dumps are
>         skipped.
>
> Since no spinlock protecting us, due to the concurrent running of
> atomic_inc_return(&core_dump_count), even with the changing above
> it's not guaranteed that core_dump_count can't exceed core_pipe_limit).
> As you said, suppose both of them are equal to INT_MAX(0x7fffffff),
> and before any dummping thread drops core_dump_count, one new thread
> comes in then hits atomic_inc_return(&core_dump_count) and now
> (unsigned int)core_dump_count is 0x80000000, but original codes checking
> for core_pipe_limit still works as expected.

You are absolutely right about this. Moreover, as stated in my commit
message, pid or memory exhaustion should occur before reaching this
scenario.

Thank's for your comment and review. I'll fix and push a v2.

>
> Please correct me if I'm wrong :)
>
> Thanks,
> linfeng
>
>