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@xxxxxxxxxxx wrote:From: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx> 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@xxxxxxxxxxx> --- 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@xxxxxxxxxx>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",
