From e7fd1549aeb83e34ee0955cdf5dee5d4088508f3 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:16 -0700 Subject: coredump: format_corename() can leak cn->corename do_coredump() assumes that format_corename() can only fail if expand_corename() fails and frees cn->corename. This is not true, for example cn_print_exe_file() can fail and in this case nobody frees cn->corename. Change do_coredump() to always do kfree(cn->corename) after it calls format_corename() (NULL is fine), change expand_corename() to do nothing if kmalloc() fails. Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index dafafba..11bc368 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -58,16 +58,14 @@ static atomic_t call_count = ATOMIC_INIT(1); static int expand_corename(struct core_name *cn) { - char *old_corename = cn->corename; + int size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count); + char *corename = krealloc(cn->corename, size, GFP_KERNEL); - cn->size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count); - cn->corename = krealloc(old_corename, cn->size, GFP_KERNEL); - - if (!cn->corename) { - kfree(old_corename); + if (!corename) return -ENOMEM; - } + cn->size = size; + cn->corename = corename; return 0; } @@ -157,10 +155,9 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) int pid_in_pattern = 0; int err = 0; + cn->used = 0; cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count); cn->corename = kmalloc(cn->size, GFP_KERNEL); - cn->used = 0; - if (!cn->corename) return -ENOMEM; @@ -549,7 +546,7 @@ void do_coredump(siginfo_t *siginfo) if (ispipe < 0) { printk(KERN_WARNING "format_corename failed\n"); printk(KERN_WARNING "Aborting core\n"); - goto fail_corename; + goto fail_unlock; } if (cprm.limit == 1) { @@ -669,7 +666,6 @@ fail_dropcount: atomic_dec(&core_dump_count); fail_unlock: kfree(cn.corename); -fail_corename: coredump_finish(mm, core_dumped); revert_creds(old_cred); fail_creds: -- cgit v1.1 From bc03c691aa86948af4e272ebdcdd4203018210f3 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:17 -0700 Subject: coredump: introduce cn_vprintf() Turn cn_printf(...) into cn_vprintf(va_list args), reintroduce cn_printf() as a trivial wrapper. This simplifies the next change and cn_vprintf() will have more callers. Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index 11bc368..c10a43a 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -69,17 +69,13 @@ static int expand_corename(struct core_name *cn) return 0; } -static int cn_printf(struct core_name *cn, const char *fmt, ...) +static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg) { char *cur; int need; int ret; - va_list arg; - va_start(arg, fmt); need = vsnprintf(NULL, 0, fmt, arg); - va_end(arg); - if (likely(need < cn->size - cn->used - 1)) goto out_printf; @@ -89,9 +85,7 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...) out_printf: cur = cn->corename + cn->used; - va_start(arg, fmt); vsnprintf(cur, need + 1, fmt, arg); - va_end(arg); cn->used += need; return 0; @@ -99,6 +93,18 @@ expand_fail: return ret; } +static int cn_printf(struct core_name *cn, const char *fmt, ...) +{ + va_list arg; + int ret; + + va_start(arg, fmt); + ret = cn_vprintf(cn, fmt, arg); + va_end(arg); + + return ret; +} + static void cn_escape(char *str) { for (; *str; str++) -- cgit v1.1 From 5fe9d8ca21cc1517258fe448639392d5d542eec6 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:19 -0700 Subject: coredump: cn_vprintf() has no reason to call vsnprintf() twice cn_vprintf() looks really overcomplicated and sub-optimal. We do not need vsnprintf(NULL) to calculate the size we need, we can simply try to print into the current buffer and expand/retry only if necessary. Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index c10a43a..2b1d1f5 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -71,26 +71,20 @@ static int expand_corename(struct core_name *cn) static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg) { - char *cur; - int need; - int ret; - - need = vsnprintf(NULL, 0, fmt, arg); - if (likely(need < cn->size - cn->used - 1)) - goto out_printf; - - ret = expand_corename(cn); - if (ret) - goto expand_fail; + int free, need; + +again: + free = cn->size - cn->used; + need = vsnprintf(cn->corename + cn->used, free, fmt, arg); + if (need < free) { + cn->used += need; + return 0; + } -out_printf: - cur = cn->corename + cn->used; - vsnprintf(cur, need + 1, fmt, arg); - cn->used += need; - return 0; + if (!expand_corename(cn)) + goto again; -expand_fail: - return ret; + return -ENOMEM; } static int cn_printf(struct core_name *cn, const char *fmt, ...) -- cgit v1.1 From 923bed030ff6e20b5176e10da151fade83097891 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:20 -0700 Subject: coredump: kill cn_escape(), introduce cn_esc_printf() The usage of cn_escape() looks really annoying, imho this sequence needs a wrapper. And it is buggy. If cn_printf() does expand_corename() cn_escape() writes to the freed memory. Introduce cn_esc_printf() which hopefully does this all right. It records the index before cn_vprintf(), not "char *" which is no longer valid (in general) after krealloc(). Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index 2b1d1f5..90d7cee 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -99,11 +99,21 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...) return ret; } -static void cn_escape(char *str) +static int cn_esc_printf(struct core_name *cn, const char *fmt, ...) { - for (; *str; str++) - if (*str == '/') - *str = '!'; + int cur = cn->used; + va_list arg; + int ret; + + va_start(arg, fmt); + ret = cn_vprintf(cn, fmt, arg); + va_end(arg); + + for (; cur < cn->used; ++cur) { + if (cn->corename[cur] == '/') + cn->corename[cur] = '!'; + } + return ret; } static int cn_print_exe_file(struct core_name *cn) @@ -113,12 +123,8 @@ static int cn_print_exe_file(struct core_name *cn) int ret; exe_file = get_mm_exe_file(current->mm); - if (!exe_file) { - char *commstart = cn->corename + cn->used; - ret = cn_printf(cn, "%s (path unknown)", current->comm); - cn_escape(commstart); - return ret; - } + if (!exe_file) + return cn_esc_printf(cn, "%s (path unknown)", current->comm); pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY); if (!pathbuf) { @@ -132,9 +138,7 @@ static int cn_print_exe_file(struct core_name *cn) goto free_buf; } - cn_escape(path); - - ret = cn_printf(cn, "%s", path); + ret = cn_esc_printf(cn, "%s", path); free_buf: kfree(pathbuf); @@ -207,22 +211,16 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) break; } /* hostname */ - case 'h': { - char *namestart = cn->corename + cn->used; + case 'h': down_read(&uts_sem); - err = cn_printf(cn, "%s", + err = cn_esc_printf(cn, "%s", utsname()->nodename); up_read(&uts_sem); - cn_escape(namestart); break; - } /* executable */ - case 'e': { - char *commstart = cn->corename + cn->used; - err = cn_printf(cn, "%s", current->comm); - cn_escape(commstart); + case 'e': + err = cn_esc_printf(cn, "%s", current->comm); break; - } case 'E': err = cn_print_exe_file(cn); break; -- cgit v1.1 From 3ceadcf6d489650ade673b7197c11c521aecb038 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:22 -0700 Subject: coredump: kill call_count, add core_name_size Imho, "atomic_t call_count" is ugly and should die. It buys nothing and in fact it can grow more than necessary, expand doesn't check if it was already incremented by another task. Kill it, and introduce "static int core_name_size" updated by expand_corename(). This is obviously racy too but harmless, and core_name_size never grows for no reason. We do not bother to to calculate the "right" new size, we simply do kmalloc(size_we_need) and use ksize() to rely on kmalloc_index's decision. Finally change format_corename() to use expand_corename(), krealloc(NULL) is fine. Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index 90d7cee..56a9ab9 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -45,26 +45,28 @@ #include int core_uses_pid; -char core_pattern[CORENAME_MAX_SIZE] = "core"; unsigned int core_pipe_limit; +char core_pattern[CORENAME_MAX_SIZE] = "core"; +static int core_name_size = CORENAME_MAX_SIZE; struct core_name { char *corename; int used, size; }; -static atomic_t call_count = ATOMIC_INIT(1); /* The maximal length of core_pattern is also specified in sysctl.c */ -static int expand_corename(struct core_name *cn) +static int expand_corename(struct core_name *cn, int size) { - int size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count); char *corename = krealloc(cn->corename, size, GFP_KERNEL); if (!corename) return -ENOMEM; - cn->size = size; + if (size > core_name_size) /* racy but harmless */ + core_name_size = size; + + cn->size = ksize(corename); cn->corename = corename; return 0; } @@ -81,7 +83,7 @@ again: return 0; } - if (!expand_corename(cn)) + if (!expand_corename(cn, cn->size + need - free + 1)) goto again; return -ENOMEM; @@ -160,9 +162,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) int err = 0; cn->used = 0; - cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count); - cn->corename = kmalloc(cn->size, GFP_KERNEL); - if (!cn->corename) + cn->corename = NULL; + if (expand_corename(cn, core_name_size)) return -ENOMEM; /* Repeat as long as we have more pattern to process and more output -- cgit v1.1 From 888ffc5923e4343a78575918ab781e85fa22d244 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 3 Jul 2013 15:08:23 -0700 Subject: coredump: '% at the end' shouldn't bypass core_uses_pid logic "goto end" should not bypass the "Backward compatibility with core_uses_pid" code, move this label up. While at it, - It is ugly to copy '|' into cn->corename and then inc the pointer for argv_split(). Change format_corename() to increment pat_ptr instead. - Remove the dead "if (*pat_ptr == 0)" in format_corename(), we already checked it is not zero. Signed-off-by: Oleg Nesterov Cc: Andi Kleen Cc: Colin Walters Cc: Denys Vlasenko Cc: Jiri Slaby Cc: Lennart Poettering Cc: Lucas De Marchi Acked-by: Neil Horman Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coredump.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'fs/coredump.c') diff --git a/fs/coredump.c b/fs/coredump.c index 56a9ab9..72f816d 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -165,13 +165,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) cn->corename = NULL; if (expand_corename(cn, core_name_size)) return -ENOMEM; + cn->corename[0] = '\0'; + + if (ispipe) + ++pat_ptr; /* Repeat as long as we have more pattern to process and more output space */ while (*pat_ptr) { if (*pat_ptr != '%') { - if (*pat_ptr == 0) - goto out; err = cn_printf(cn, "%c", *pat_ptr++); } else { switch (*++pat_ptr) { @@ -240,6 +242,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) return err; } +out: /* Backward compatibility with core_uses_pid: * * If core_pattern does not include a %p (as is the default) @@ -250,7 +253,6 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) if (err) return err; } -out: return ispipe; } @@ -580,7 +582,7 @@ void do_coredump(siginfo_t *siginfo) goto fail_dropcount; } - helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL); + helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL); if (!helper_argv) { printk(KERN_WARNING "%s failed to allocate memory\n", __func__); @@ -597,7 +599,7 @@ void do_coredump(siginfo_t *siginfo) argv_free(helper_argv); if (retval) { - printk(KERN_INFO "Core dump to %s pipe failed\n", + printk(KERN_INFO "Core dump to |%s pipe failed\n", cn.corename); goto close_fail; } -- cgit v1.1