[RFC PATCH] core_pattern: fix long parameters was truncated by core_pattern handler

[RFC PATCH] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g %t 11 1234567890123456789012345678901234567890" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
	argc[10]=<12345678901234567890123456789012345678>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE.

This patch expands the size of parsing array, and makes the cursor
out_end shift when we replace % specifiers.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index e19de6a..e67e4b5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1441,7 +1441,7 @@ EXPORT_SYMBOL(set_binfmt);
 
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
- * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
+ * CORENAME_MAX_SIZE * 3 bytes because of % specifiers.
  */
 static int format_corename(char *corename, long signr)
 {
@@ -1449,7 +1449,7 @@ static int format_corename(char *corename, long signr)
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
 	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
+	char *out_end = corename + CORENAME_MAX_SIZE;
 	int rc;
 	int pid_in_pattern = 0;
 
@@ -1478,6 +1478,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* uid */
 			case 'u':
@@ -1486,6 +1487,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* gid */
 			case 'g':
@@ -1494,6 +1496,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* signal that caused the coredump */
 			case 's':
@@ -1502,6 +1505,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* UNIX time of coredump */
 			case 't': {
@@ -1512,6 +1516,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			}
 			/* hostname */
@@ -1523,6 +1528,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* executable */
 			case 'e':
@@ -1531,6 +1537,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			/* core limit size */
 			case 'c':
@@ -1539,6 +1546,7 @@ static int format_corename(char *corename, long signr)
 				if (rc > out_end - out_ptr)
 					goto out;
 				out_ptr += rc;
+				out_end += rc - 2;
 				break;
 			default:
 				break;
@@ -1836,7 +1844,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	char corename[CORENAME_MAX_SIZE * 3];
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
-- 
1.7.2

Re: [RFC PATCH] core_pattern: fix long parameters was truncated by core_pattern handler

[Neil Horman]

On Thu, Jul 29, 2010 at 08:42:44PM +0800, Xiaotian Feng wrote:
> We met a parameter truncated issue, consider following:
> > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> %s %c %p %u %g %t 11 1234567890123456789012345678901234567890" > \
> /proc/sys/kernel/core_pattern
> 
> This is okay because the strings is less than CORENAME_MAX_SIZE.
> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> after we run core_pattern_pipe_test in man page, we found last
> parameter was truncated like below:
> 	argc[10]=<12345678901234567890123456789012345678>
> 
> The root cause is core_pattern allows % specifiers, which need to be
> replaced during parse time, but the replace may expand the strings
> to larger than CORENAME_MAX_SIZE.
> 
> This patch expands the size of parsing array, and makes the cursor
> out_end shift when we replace % specifiers.
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Roland McGrath <roland@redhat.com>
> ---
Thanks for looking at this, its definately a problem.  I don't think though,
that just extending the size of the corename array to be bigger is really going
to solve the problem, you're just running away from it.  To really fix it what
we should probably do is:
1) Modify the corename argument to format_corename to be char **corename

2) Dynamically allocate an array for corename inside format_corename, of size
CORENAME_MAX_SIZE*n, where n is variable holding the maximum number of times we
had to increment our allocation size in any previous call to format_corename

3) for each iteration through the while (*pat_ptr) loop in format_corename,
check to see if the remaining size can hold the next argument to be parsed.  If
we're going to overrun, use krealloc to extend the size of the array to another
multiple or CORENAME_MAX_SIZE, and increment the variable n in (2) by 1.


That should give us a decent heuristic to determine the size of the corename
array, so that we never overrun.

Regards
Neil

>  fs/exec.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index e19de6a..e67e4b5 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1441,7 +1441,7 @@ EXPORT_SYMBOL(set_binfmt);
>  
>  /* format_corename will inspect the pattern parameter, and output a
>   * name into corename, which must have space for at least
> - * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
> + * CORENAME_MAX_SIZE * 3 bytes because of % specifiers.
>   */
>  static int format_corename(char *corename, long signr)
>  {
> @@ -1449,7 +1449,7 @@ static int format_corename(char *corename, long signr)
>  	const char *pat_ptr = core_pattern;
>  	int ispipe = (*pat_ptr == '|');
>  	char *out_ptr = corename;
> -	char *const out_end = corename + CORENAME_MAX_SIZE;
> +	char *out_end = corename + CORENAME_MAX_SIZE;
>  	int rc;
>  	int pid_in_pattern = 0;
>  
> @@ -1478,6 +1478,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* uid */
>  			case 'u':
> @@ -1486,6 +1487,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* gid */
>  			case 'g':
> @@ -1494,6 +1496,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* signal that caused the coredump */
>  			case 's':
> @@ -1502,6 +1505,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* UNIX time of coredump */
>  			case 't': {
> @@ -1512,6 +1516,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			}
>  			/* hostname */
> @@ -1523,6 +1528,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* executable */
>  			case 'e':
> @@ -1531,6 +1537,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			/* core limit size */
>  			case 'c':
> @@ -1539,6 +1546,7 @@ static int format_corename(char *corename, long signr)
>  				if (rc > out_end - out_ptr)
>  					goto out;
>  				out_ptr += rc;
> +				out_end += rc - 2;
>  				break;
>  			default:
>  				break;
> @@ -1836,7 +1844,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
>  void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  {
>  	struct core_state core_state;
> -	char corename[CORENAME_MAX_SIZE + 1];
> +	char corename[CORENAME_MAX_SIZE * 3];
>  	struct mm_struct *mm = current->mm;
>  	struct linux_binfmt * binfmt;
>  	const struct cred *old_cred;
> -- 
> 1.7.2
> 
> 

[RFC PATCH V2] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
        argc[10]=<12807486>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE. So if the last parameter is %
specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
this will write out of corename array.

This patch allocates corename at runtime, if the replace doesn't have enough
memory, expand the corename dynamically.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |  194 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 142 insertions(+), 52 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index e19de6a..d9e1e4f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1439,26 +1439,56 @@ void set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static int expand_corename(char **corename, char **out_end,
+			   char **out_ptr, int *size)
+{
+	unsigned long ptr_offset;
+	char *old_corename = *corename;
+
+	(*size)++;
+	ptr_offset = *out_ptr - *corename;
+	*corename = krealloc(*corename, *size * CORENAME_MAX_SIZE,
+			     GFP_KERNEL);
+
+	if (*corename == NULL) {
+		kfree(old_corename);
+		return -ENOMEM;
+	}
+
+	*out_end = *corename + *size * CORENAME_MAX_SIZE - 1;
+	*out_ptr = *corename + ptr_offset;
+	return 0;
+}
 /* format_corename will inspect the pattern parameter, and output a
- * name into corename, which must have space for at least
- * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
+ * name into corename, which will be allocated at runtime.
  */
-static int format_corename(char *corename, long signr)
+static int format_corename(char **corename, long signr)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
-	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
-	int rc;
+	char *out_ptr, *out_end, *old_corename;
+	int size = 1;
+	int rc, ret;
 	int pid_in_pattern = 0;
 
+	*corename = kmalloc(CORENAME_MAX_SIZE, GFP_KERNEL);
+	if (*corename == NULL)
+		return -ENOMEM;
+
+	old_corename = *corename;
+	out_ptr = *corename;
+	out_end = *corename + CORENAME_MAX_SIZE - 1;
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (out_ptr == out_end)
-				goto out;
+			if (out_ptr == out_end) {
+				ret = expand_corename(corename, &out_end,
+						      &out_ptr, &size);
+				if (ret)
+					return ret;
+			}
 			*out_ptr++ = *pat_ptr++;
 		} else {
 			switch (*++pat_ptr) {
@@ -1466,78 +1496,126 @@ static int format_corename(char *corename, long signr)
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (out_ptr == out_end)
-					goto out;
 				*out_ptr++ = '%';
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", task_tgid_vnr(current));
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%d",
+					      task_tgid_vnr(current));
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%d",
+					      task_tgid_vnr(current));
 				out_ptr += rc;
 				break;
 			/* uid */
 			case 'u':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->uid);
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%d", cred->uid);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%d", cred->uid);
 				out_ptr += rc;
 				break;
 			/* gid */
 			case 'g':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->gid);
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%d", cred->gid);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%d", cred->gid);
 				out_ptr += rc;
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%ld", signr);
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%ld", signr);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret == -ENOMEM)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%ld", signr);
 				out_ptr += rc;
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", tv.tv_sec);
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%lu", tv.tv_sec);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%lu",
+					      tv.tv_sec);
 				out_ptr += rc;
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", utsname()->nodename);
+				rc = snprintf(NULL, 0, "%s",
+					      utsname()->nodename);
+				up_read(&uts_sem);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				down_read(&uts_sem);
+				rc = snprintf(out_ptr, rc + 1, "%s",
+					      utsname()->nodename);
 				up_read(&uts_sem);
-				if (rc > out_end - out_ptr)
-					goto out;
 				out_ptr += rc;
 				break;
 			/* executable */
 			case 'e':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", current->comm);
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%s", current->comm);
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%s",
+					      current->comm);
 				out_ptr += rc;
 				break;
 			/* core limit size */
 			case 'c':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", rlimit(RLIMIT_CORE));
-				if (rc > out_end - out_ptr)
-					goto out;
+				rc = snprintf(NULL, 0, "%lu",
+					      rlimit(RLIMIT_CORE));
+				if (rc > out_end - out_ptr) {
+					ret = expand_corename(corename,
+							      &out_end,
+							      &out_ptr, &size);
+					if (ret == -ENOMEM)
+						return ret;
+				}
+				rc = snprintf(out_ptr, rc + 1, "%lu",
+					      rlimit(RLIMIT_CORE));
 				out_ptr += rc;
 				break;
 			default:
@@ -1552,10 +1630,14 @@ static int format_corename(char *corename, long signr)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
 	if (!ispipe && !pid_in_pattern && core_uses_pid) {
-		rc = snprintf(out_ptr, out_end - out_ptr,
-			      ".%d", task_tgid_vnr(current));
-		if (rc > out_end - out_ptr)
-			goto out;
+		rc = snprintf(NULL, 0, ".%d", task_tgid_vnr(current));
+		if (rc > out_end - out_ptr) {
+			ret = expand_corename(corename, &out_end, &out_ptr,
+					      &size);
+			if (ret == -ENOMEM)
+				return ret;
+		}
+		rc = snprintf(out_ptr, rc + 1, "%d", task_tgid_vnr(current));
 		out_ptr += rc;
 	}
 out:
@@ -1836,7 +1918,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	char *corename;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
@@ -1895,11 +1977,17 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 * lock_kernel() because format_corename() is controlled by sysctl, which
 	 * uses lock_kernel()
 	 */
- 	lock_kernel();
-	ispipe = format_corename(corename, signr);
+	lock_kernel();
+	ispipe = format_corename(&corename, signr);
 	unlock_kernel();
 
- 	if (ispipe) {
+	if (ispipe == -ENOMEM) {
+		printk(KERN_WARNING "format_corename failed\n");
+		printk(KERN_WARNING "Aborting core\n");
+		goto fail_corename;
+	}
+
+	if (ispipe) {
 		int dump_count;
 		char **helper_argv;
 
@@ -1946,10 +2034,10 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 					NULL, &cprm);
 		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",
 			       corename);
 			goto close_fail;
- 		}
+		}
 	} else {
 		struct inode *inode;
 
@@ -1998,6 +2086,8 @@ fail_dropcount:
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(corename);
+fail_corename:
 	coredump_finish(mm);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.7.2

Re: [RFC PATCH V2] core_pattern: fix long parameters was truncated by core_pattern handler

[Oleg Nesterov]

On 08/02, Xiaotian Feng wrote:
>
> @@ -1466,78 +1496,126 @@ static int format_corename(char *corename, long signr)
>  				goto out;
>  			/* Double percent, output one percent */
>  			case '%':
> -				if (out_ptr == out_end)
> -					goto out;
>  				*out_ptr++ = '%';

Hmm. Not sure I understand why we do not need to check the space here.

> -				rc = snprintf(out_ptr, out_end - out_ptr,
> -					      "%d", task_tgid_vnr(current));
> -				if (rc > out_end - out_ptr)
> -					goto out;
> +				rc = snprintf(NULL, 0, "%d",
> +					      task_tgid_vnr(current));
> +				if (rc > out_end - out_ptr) {
> +					ret = expand_corename(corename,
> +							      &out_end,
> +							      &out_ptr, &size);
> +					if (ret)
> +						return ret;
> +				}
> +				rc = snprintf(out_ptr, rc + 1, "%d",
> +					      task_tgid_vnr(current));

Probably it makes sense to factor out this code?

Roughly, something like:

	struct core_name {
		char *corename;
		int len, free;
	};

	static bool cn_printf(struct core_name *cn, const char *fmt, ...)
	{
		char *cur;
		int need;
		va_list ap;

	retry:
		cur = cn->corename + (cn->len - cn->free);
		need = vsnprintf(cur, cn->free, fmt, ap);

		if (likely(need < free)) {
			free -= need;
			return true;
		}

		increase ->len, realloc ->corename or return false;
		goto retry;

	}


Then format_corename() can just do

	if (!cn_printf(&cn, ...))
		return -ENOMEM;

consistently.



Also. Not sure this really makes sense, but if we ever need to expand the
string, perhaps it makes sense to remeber this fact so that the next time
we start with len > CORENAME_MAX_SIZE. In any case, I think this needs a
separate patch.

Oleg.

Re: [RFC PATCH V2] core_pattern: fix long parameters was truncated by core_pattern handler

[Denys Vlasenko]

> +                               rc = snprintf(NULL, 0, "%s", current->comm);

rc = strlen(current->comm) would be smaller

-- 
vda

Re: [RFC PATCH V2] core_pattern: fix long parameters was truncated by core_pattern handler

[Denys Vlasenko]

> +                               rc = snprintf(NULL, 0, "%s", current->comm);

rc = strlen(current->comm) would be smaller

-- 
vda
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH V2] core_pattern: fix long parameters was truncated by core_pattern handler

[Neil Horman]

On Mon, Aug 02, 2010 at 03:50:13PM +0200, Oleg Nesterov wrote:
> 
> Also. Not sure this really makes sense, but if we ever need to expand the
> string, perhaps it makes sense to remeber this fact so that the next time
> we start with len > CORENAME_MAX_SIZE. In any case, I think this needs a
> separate patch.
> 
> Oleg.

Yes, this is what I alluded to in my initial reply.  We need to keep an atomic
value to track the maximum number of times we've called expand_corename to help
prevent us having to call it repeatedly on every crash.  Something like this:

expand_corename(**corename, **out_end, **out_ptr)
{
	...
	*corename = krealloc(*corename,
			     CORENAME_MAX_SIZE*
			     atomic_inc_return(call_count),
			     GFP_KERNEL);
	...
}


do_coredump(...)
{
	...
	corename = kmalloc(CORENAME_MAX_SIZE *
			   atomic_read(call_count),
			   GFP_KERNEL);
	...
}


That will train the path to allocate a sensible size after the first crash, and
avoid lots of calls to krealloc in the pessimal case

Neil

[RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
        argc[10]=<12807486>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE. So if the last parameter is %
specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
this will write out of corename array.

Changes since v2:
Introduced generic function cn_printf and make format_corename remember the time
has been expanded.

Changes since v1:
This patch allocates corename at runtime, if the replace doesn't have enough
memory, expand the corename dynamically.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |  181 +++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 121 insertions(+), 60 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 2d94552..e2fe568 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -65,6 +65,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
 int suid_dumpable = 0;
 
+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 LIST_HEAD(formats);
@@ -1440,106 +1446,147 @@ void set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static int expand_corename(struct core_name *cn)
+{
+	char *old_corename = cn->corename;
+
+	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);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int cn_printf(struct core_name *cn, const char *fmt, ...)
+{
+	char *cur;
+	int need;
+	int ret;
+	va_list arg;
+
+	cur = cn->corename + cn->used;
+
+	va_start(arg, fmt);
+	need = vsnprintf(NULL, 0, fmt, arg);
+	va_end(arg);
+
+	if (likely(need < cn->size - cn->used))
+		goto out_printf;
+
+	ret = expand_corename(cn);
+	if (ret)
+		goto expand_fail;
+
+out_printf:
+	va_start(arg, fmt);
+	vsnprintf(cur, need + 1, fmt, arg);
+	va_end(arg);
+	cn->used += need;
+	return 0;
+
+expand_fail:
+	va_end(arg);
+	return ret;
+}
+
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(char *corename, long signr)
+static int format_corename(struct core_name *cn, long signr)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
-	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
-	int rc;
+	char *out_ptr;
 	int pid_in_pattern = 0;
 
+	cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count);
+	cn->corename = kmalloc(cn->size, GFP_KERNEL);
+	cn->used = 0;
+
+	if (!cn->corename)
+		goto out_fail;
+
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (out_ptr == out_end)
-				goto out;
-			*out_ptr++ = *pat_ptr++;
+			if (cn->used == cn->size)
+				if (expand_corename(cn))
+					goto out_fail;
+
+			out_ptr = cn->corename + cn->used;
+			*out_ptr = *pat_ptr++;
+			cn->used++;
 		} else {
 			switch (*++pat_ptr) {
 			case 0:
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (out_ptr == out_end)
-					goto out;
-				*out_ptr++ = '%';
+				if (cn->used == cn->size)
+					if (expand_corename(cn))
+						goto out_fail;
+
+				out_ptr = cn->corename + cn->used;
+				*out_ptr = '%';
+				cn->used++;
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", task_tgid_vnr(current));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d",
+					      task_tgid_vnr(current)))
+					goto out_fail;
 				break;
 			/* uid */
 			case 'u':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->uid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d", cred->uid))
+					goto out_fail;
 				break;
 			/* gid */
 			case 'g':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->gid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d", cred->gid))
+					goto out_fail;
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%ld", signr);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%ld", signr))
+					goto out_fail;
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", tv.tv_sec);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%lu", tv.tv_sec))
+					goto out_fail;
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", utsname()->nodename);
+				if (cn_printf(cn, "%s",
+					      utsname()->nodename)) {
+					up_read(&uts_sem);
+					goto out_fail;
+				}
 				up_read(&uts_sem);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
 				break;
 			/* executable */
 			case 'e':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", current->comm);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%s", current->comm))
+					goto out_fail;
 				break;
 			/* core limit size */
 			case 'c':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", rlimit(RLIMIT_CORE));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%lu",
+					      rlimit(RLIMIT_CORE)))
+					goto out_fail;
 				break;
 			default:
 				break;
@@ -1553,15 +1600,21 @@ static int format_corename(char *corename, long signr)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
 	if (!ispipe && !pid_in_pattern && core_uses_pid) {
-		rc = snprintf(out_ptr, out_end - out_ptr,
-			      ".%d", task_tgid_vnr(current));
-		if (rc > out_end - out_ptr)
-			goto out;
-		out_ptr += rc;
+		if (cn_printf(cn, ".%d", task_tgid_vnr(current)))
+			goto out_fail;
 	}
 out:
+	out_ptr = cn->corename + cn->used;
+	if (cn->used == cn->size)
+		if (expand_corename(cn))
+			goto out_fail;
+
+	out_ptr = cn->corename + cn->used;
 	*out_ptr = 0;
 	return ispipe;
+
+out_fail:
+	return -ENOMEM;
 }
 
 static int zap_process(struct task_struct *start, int exit_code)
@@ -1837,7 +1890,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	struct core_name cn;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
@@ -1892,7 +1945,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	ispipe = format_corename(corename, signr);
+	ispipe = format_corename(&cn, signr);
+
+	if (ispipe == -ENOMEM) {
+		printk(KERN_WARNING "format_corename failed\n");
+		printk(KERN_WARNING "Aborting core\n");
+		goto fail_corename;
+	}
 
  	if (ispipe) {
 		int dump_count;
@@ -1929,7 +1988,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, corename+1, NULL);
+		helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
@@ -1942,7 +2001,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		argv_free(helper_argv);
 		if (retval) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
-			       corename);
+			       cn.corename);
 			goto close_fail;
  		}
 	} else {
@@ -1951,7 +2010,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		if (cprm.limit < binfmt->min_coredump)
 			goto fail_unlock;
 
-		cprm.file = filp_open(corename,
+		cprm.file = filp_open(cn.corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
 		if (IS_ERR(cprm.file))
@@ -1993,6 +2052,8 @@ fail_dropcount:
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(cn.corename);
+fail_corename:
 	coredump_finish(mm);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.7.2.1

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

On 08/20/2010 05:22 PM, Xiaotian Feng wrote:
> We met a parameter truncated issue, consider following:
>> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t">  \
> /proc/sys/kernel/core_pattern
>
> This is okay because the strings is less than CORENAME_MAX_SIZE.
> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> after we run core_pattern_pipe_test in man page, we found last
> parameter was truncated like below:
>          argc[10]=<12807486>
>
> The root cause is core_pattern allows % specifiers, which need to be
> replaced during parse time, but the replace may expand the strings
> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
> this will write out of corename array.
>
> Changes since v2:
> Introduced generic function cn_printf and make format_corename remember the time
> has been expanded.
>
> Changes since v1:
> This patch allocates corename at runtime, if the replace doesn't have enough
> memory, expand the corename dynamically.
>
> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
> Cc: Alexander Viro<viro@zeniv.linux.org.uk>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Cc: Oleg Nesterov<oleg@redhat.com>
> Cc: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> Cc: Neil Horman<nhorman@tuxdriver.com>
> Cc: Roland McGrath<roland@redhat.com>
> ---
>   fs/exec.c |  181 +++++++++++++++++++++++++++++++++++++++++--------------------
>   1 files changed, 121 insertions(+), 60 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 2d94552..e2fe568 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -65,6 +65,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
>   unsigned int core_pipe_limit;
>   int suid_dumpable = 0;
>
> +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 LIST_HEAD(formats);
> @@ -1440,106 +1446,147 @@ void set_binfmt(struct linux_binfmt *new)
>
>   EXPORT_SYMBOL(set_binfmt);
>
> +static int expand_corename(struct core_name *cn)
> +{
> +	char *old_corename = cn->corename;
> +
> +	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);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cn_printf(struct core_name *cn, const char *fmt, ...)
> +{
> +	char *cur;
> +	int need;
> +	int ret;
> +	va_list arg;
> +
> +	cur = cn->corename + cn->used;
> +
> +	va_start(arg, fmt);
> +	need = vsnprintf(NULL, 0, fmt, arg);
> +	va_end(arg);
> +
> +	if (likely(need<  cn->size - cn->used))
> +		goto out_printf;
> +
> +	ret = expand_corename(cn);
> +	if (ret)
> +		goto expand_fail;
> +
> +out_printf:
> +	va_start(arg, fmt);
> +	vsnprintf(cur, need + 1, fmt, arg);
> +	va_end(arg);
> +	cn->used += need;
> +	return 0;
> +
> +expand_fail:
> +	va_end(arg);

oops, this line should be removed, please ignore this mail, I'll send an 
updated patch.

Thanks
Xiaotian

[RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
        argc[10]=<12807486>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE. So if the last parameter is %
specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
this will write out of corename array.

Changes since v2:
Introduced generic function cn_printf and make format_corename remember the time
has been expanded.

Changes since v1:
This patch allocates corename at runtime, if the replace doesn't have enough
memory, expand the corename dynamically.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |  180 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 120 insertions(+), 60 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 2d94552..d21c8c0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -65,6 +65,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
 int suid_dumpable = 0;
 
+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 LIST_HEAD(formats);
@@ -1440,106 +1446,146 @@ void set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static int expand_corename(struct core_name *cn)
+{
+	char *old_corename = cn->corename;
+
+	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);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int cn_printf(struct core_name *cn, const char *fmt, ...)
+{
+	char *cur;
+	int need;
+	int ret;
+	va_list arg;
+
+	cur = cn->corename + cn->used;
+
+	va_start(arg, fmt);
+	need = vsnprintf(NULL, 0, fmt, arg);
+	va_end(arg);
+
+	if (likely(need < cn->size - cn->used))
+		goto out_printf;
+
+	ret = expand_corename(cn);
+	if (ret)
+		goto expand_fail;
+
+out_printf:
+	va_start(arg, fmt);
+	vsnprintf(cur, need + 1, fmt, arg);
+	va_end(arg);
+	cn->used += need;
+	return 0;
+
+expand_fail:
+	return ret;
+}
+
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(char *corename, long signr)
+static int format_corename(struct core_name *cn, long signr)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
-	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
-	int rc;
+	char *out_ptr;
 	int pid_in_pattern = 0;
 
+	cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count);
+	cn->corename = kmalloc(cn->size, GFP_KERNEL);
+	cn->used = 0;
+
+	if (!cn->corename)
+		goto out_fail;
+
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (out_ptr == out_end)
-				goto out;
-			*out_ptr++ = *pat_ptr++;
+			if (cn->used == cn->size)
+				if (expand_corename(cn))
+					goto out_fail;
+
+			out_ptr = cn->corename + cn->used;
+			*out_ptr = *pat_ptr++;
+			cn->used++;
 		} else {
 			switch (*++pat_ptr) {
 			case 0:
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (out_ptr == out_end)
-					goto out;
-				*out_ptr++ = '%';
+				if (cn->used == cn->size)
+					if (expand_corename(cn))
+						goto out_fail;
+
+				out_ptr = cn->corename + cn->used;
+				*out_ptr = '%';
+				cn->used++;
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", task_tgid_vnr(current));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d",
+					      task_tgid_vnr(current)))
+					goto out_fail;
 				break;
 			/* uid */
 			case 'u':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->uid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d", cred->uid))
+					goto out_fail;
 				break;
 			/* gid */
 			case 'g':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->gid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%d", cred->gid))
+					goto out_fail;
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%ld", signr);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%ld", signr))
+					goto out_fail;
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", tv.tv_sec);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%lu", tv.tv_sec))
+					goto out_fail;
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", utsname()->nodename);
+				if (cn_printf(cn, "%s",
+					      utsname()->nodename)) {
+					up_read(&uts_sem);
+					goto out_fail;
+				}
 				up_read(&uts_sem);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
 				break;
 			/* executable */
 			case 'e':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", current->comm);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%s", current->comm))
+					goto out_fail;
 				break;
 			/* core limit size */
 			case 'c':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", rlimit(RLIMIT_CORE));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				if (cn_printf(cn, "%lu",
+					      rlimit(RLIMIT_CORE)))
+					goto out_fail;
 				break;
 			default:
 				break;
@@ -1553,15 +1599,21 @@ static int format_corename(char *corename, long signr)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
 	if (!ispipe && !pid_in_pattern && core_uses_pid) {
-		rc = snprintf(out_ptr, out_end - out_ptr,
-			      ".%d", task_tgid_vnr(current));
-		if (rc > out_end - out_ptr)
-			goto out;
-		out_ptr += rc;
+		if (cn_printf(cn, ".%d", task_tgid_vnr(current)))
+			goto out_fail;
 	}
 out:
+	out_ptr = cn->corename + cn->used;
+	if (cn->used == cn->size)
+		if (expand_corename(cn))
+			goto out_fail;
+
+	out_ptr = cn->corename + cn->used;
 	*out_ptr = 0;
 	return ispipe;
+
+out_fail:
+	return -ENOMEM;
 }
 
 static int zap_process(struct task_struct *start, int exit_code)
@@ -1837,7 +1889,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	struct core_name cn;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
@@ -1892,7 +1944,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	ispipe = format_corename(corename, signr);
+	ispipe = format_corename(&cn, signr);
+
+	if (ispipe == -ENOMEM) {
+		printk(KERN_WARNING "format_corename failed\n");
+		printk(KERN_WARNING "Aborting core\n");
+		goto fail_corename;
+	}
 
  	if (ispipe) {
 		int dump_count;
@@ -1929,7 +1987,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, corename+1, NULL);
+		helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
@@ -1942,7 +2000,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		argv_free(helper_argv);
 		if (retval) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
-			       corename);
+			       cn.corename);
 			goto close_fail;
  		}
 	} else {
@@ -1951,7 +2009,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		if (cprm.limit < binfmt->min_coredump)
 			goto fail_unlock;
 
-		cprm.file = filp_open(corename,
+		cprm.file = filp_open(cn.corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
 		if (IS_ERR(cprm.file))
@@ -1993,6 +2051,8 @@ fail_dropcount:
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(cn.corename);
+fail_corename:
 	coredump_finish(mm);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.7.2.1

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Neil Horman]

On Fri, Aug 20, 2010 at 05:35:58PM +0800, Xiaotian Feng wrote:
> We met a parameter truncated issue, consider following:
> > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
> /proc/sys/kernel/core_pattern
> 
> This is okay because the strings is less than CORENAME_MAX_SIZE.
> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> after we run core_pattern_pipe_test in man page, we found last
> parameter was truncated like below:
>         argc[10]=<12807486>
> 
> The root cause is core_pattern allows % specifiers, which need to be
> replaced during parse time, but the replace may expand the strings
> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
> this will write out of corename array.
> 
> Changes since v2:
> Introduced generic function cn_printf and make format_corename remember the time
> has been expanded.
> 
> Changes since v1:
> This patch allocates corename at runtime, if the replace doesn't have enough
> memory, expand the corename dynamically.
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Roland McGrath <roland@redhat.com>
> ---
>  fs/exec.c |  180 ++++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 120 insertions(+), 60 deletions(-)
> 
This looks alot cleaner.  Thanks!
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Andrew Morton]

On Fri, 20 Aug 2010 17:35:58 +0800
Xiaotian Feng <dfeng@redhat.com> wrote:

> We met a parameter truncated issue, consider following:
> > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
> /proc/sys/kernel/core_pattern
> 
> This is okay because the strings is less than CORENAME_MAX_SIZE.
> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> after we run core_pattern_pipe_test in man page, we found last
> parameter was truncated like below:
>         argc[10]=<12807486>
> 
> The root cause is core_pattern allows % specifiers, which need to be
> replaced during parse time, but the replace may expand the strings
> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
> this will write out of corename array.
> 
> Changes since v2:
> Introduced generic function cn_printf and make format_corename remember the time
> has been expanded.
> 
> Changes since v1:
> This patch allocates corename at runtime, if the replace doesn't have enough
> memory, expand the corename dynamically.


> +			if (cn->used == cn->size)
> +				if (expand_corename(cn))
> +					goto out_fail;
> +
> +			out_ptr = cn->corename + cn->used;
> +			*out_ptr = *pat_ptr++;
> +			cn->used++;


> -				if (out_ptr == out_end)
> -					goto out;
> -				*out_ptr++ = '%';
> +				if (cn->used == cn->size)
> +					if (expand_corename(cn))
> +						goto out_fail;
> +
> +				out_ptr = cn->corename + cn->used;
> +				*out_ptr = '%';
> +				cn->used++;


> +	out_ptr = cn->corename + cn->used;
> +	if (cn->used == cn->size)
> +		if (expand_corename(cn))
> +			goto out_fail;
> +
> +	out_ptr = cn->corename + cn->used;
>  	*out_ptr = 0;

Quite a bit of code duplication there.  A little helper function which
adds a single char to the output would tidy that up.

However I think that if the % and %% handers are converted to call
cn_printf() then the output is always null-terninated and the third
hunk of code above simply becomes unneeded?

Something like this, although I didn't try very hard.  Just a
suggestion to work with ;)



 fs/exec.c |   63 ++++++++++++++--------------------------------------
 1 file changed, 17 insertions(+), 46 deletions(-)

diff -puN fs/exec.c~core_pattern-fix-long-parameters-was-truncated-by-core_pattern-handler-fix fs/exec.c
--- a/fs/exec.c~core_pattern-fix-long-parameters-was-truncated-by-core_pattern-handler-fix
+++ a/fs/exec.c
@@ -1503,89 +1503,66 @@ static int format_corename(struct core_n
 	int ispipe = (*pat_ptr == '|');
 	char *out_ptr;
 	int pid_in_pattern = 0;
+	int err = 0;
 
 	cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count);
 	cn->corename = kmalloc(cn->size, GFP_KERNEL);
 	cn->used = 0;
 
 	if (!cn->corename)
-		goto out_fail;
+		return -ENOMEM;
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (cn->used == cn->size)
-				if (expand_corename(cn))
-					goto out_fail;
-
-			out_ptr = cn->corename + cn->used;
-			*out_ptr = *pat_ptr++;
-			cn->used++;
+			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
 			switch (*++pat_ptr) {
 			case 0:
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (cn->used == cn->size)
-					if (expand_corename(cn))
-						goto out_fail;
-
-				out_ptr = cn->corename + cn->used;
-				*out_ptr = '%';
-				cn->used++;
+				err = cn_printf(cn, "%%");
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				if (cn_printf(cn, "%d",
-					      task_tgid_vnr(current)))
-					goto out_fail;
+				err = cn_printf(cn, "%d",
+					      task_tgid_vnr(current));
 				break;
 			/* uid */
 			case 'u':
-				if (cn_printf(cn, "%d", cred->uid))
-					goto out_fail;
+				err = cn_printf(cn, "%d", cred->uid);
 				break;
 			/* gid */
 			case 'g':
-				if (cn_printf(cn, "%d", cred->gid))
-					goto out_fail;
+				err = cn_printf(cn, "%d", cred->gid);
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				if (cn_printf(cn, "%ld", signr))
-					goto out_fail;
+				err = cn_printf(cn, "%ld", signr);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				if (cn_printf(cn, "%lu", tv.tv_sec))
-					goto out_fail;
+				err = cn_printf(cn, "%lu", tv.tv_sec);
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				if (cn_printf(cn, "%s",
-					      utsname()->nodename)) {
-					up_read(&uts_sem);
-					goto out_fail;
-				}
+				err = cn_printf(cn, "%s", utsname()->nodename);
 				up_read(&uts_sem);
 				break;
 			/* executable */
 			case 'e':
-				if (cn_printf(cn, "%s", current->comm))
-					goto out_fail;
+				err = cn_printf(cn, "%s", current->comm);
 				break;
 			/* core limit size */
 			case 'c':
-				if (cn_printf(cn, "%lu",
-					      rlimit(RLIMIT_CORE)))
-					goto out_fail;
+				err = cn_printf(cn, "%lu", rlimit(RLIMIT_CORE));
 				break;
 			default:
 				break;
@@ -1593,6 +1570,10 @@ static int format_corename(struct core_n
 			++pat_ptr;
 		}
 	}
+
+	if (err)
+		return err;
+
 	/* Backward compatibility with core_uses_pid:
 	 *
 	 * If core_pattern does not include a %p (as is the default)
@@ -1603,17 +1584,7 @@ static int format_corename(struct core_n
 			goto out_fail;
 	}
 out:
-	out_ptr = cn->corename + cn->used;
-	if (cn->used == cn->size)
-		if (expand_corename(cn))
-			goto out_fail;
-
-	out_ptr = cn->corename + cn->used;
-	*out_ptr = 0;
 	return ispipe;
-
-out_fail:
-	return -ENOMEM;
 }
 
 static int zap_process(struct task_struct *start, int exit_code)
_

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[KOSAKI Motohiro]

> On Fri, Aug 20, 2010 at 05:35:58PM +0800, Xiaotian Feng wrote:
> > We met a parameter truncated issue, consider following:
> > > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> > %s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
> > /proc/sys/kernel/core_pattern
> > 
> > This is okay because the strings is less than CORENAME_MAX_SIZE.
> > "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> > after we run core_pattern_pipe_test in man page, we found last
> > parameter was truncated like below:
> >         argc[10]=<12807486>
> > 
> > The root cause is core_pattern allows % specifiers, which need to be
> > replaced during parse time, but the replace may expand the strings
> > to larger than CORENAME_MAX_SIZE. So if the last parameter is %
> > specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
> > this will write out of corename array.
> > 
> > Changes since v2:
> > Introduced generic function cn_printf and make format_corename remember the time
> > has been expanded.
> > 
> > Changes since v1:
> > This patch allocates corename at runtime, if the replace doesn't have enough
> > memory, expand the corename dynamically.
> > 
> > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> > Cc: Roland McGrath <roland@redhat.com>
> > ---
> >  fs/exec.c |  180 ++++++++++++++++++++++++++++++++++++++++--------------------
> >  1 files changed, 120 insertions(+), 60 deletions(-)
> > 
> This looks alot cleaner.  Thanks!
> Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
> 

Unfortunatelly, I have no time to review deeply this one. but I see my test
works. Also, I'd like to say I like this one.

Thanks.

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

On 08/24/2010 05:18 AM, Andrew Morton wrote:
> On Fri, 20 Aug 2010 17:35:58 +0800
> Xiaotian Feng<dfeng@redhat.com>  wrote:
>
>> We met a parameter truncated issue, consider following:
>>> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
>> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t">  \
>> /proc/sys/kernel/core_pattern
>>
>> This is okay because the strings is less than CORENAME_MAX_SIZE.
>> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
>> after we run core_pattern_pipe_test in man page, we found last
>> parameter was truncated like below:
>>          argc[10]=<12807486>
>>
>> The root cause is core_pattern allows % specifiers, which need to be
>> replaced during parse time, but the replace may expand the strings
>> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
>> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
>> this will write out of corename array.
>>
>> Changes since v2:
>> Introduced generic function cn_printf and make format_corename remember the time
>> has been expanded.
>>
>> Changes since v1:
>> This patch allocates corename at runtime, if the replace doesn't have enough
>> memory, expand the corename dynamically.
>
>
>> +			if (cn->used == cn->size)
>> +				if (expand_corename(cn))
>> +					goto out_fail;
>> +
>> +			out_ptr = cn->corename + cn->used;
>> +			*out_ptr = *pat_ptr++;
>> +			cn->used++;
>
>
>> -				if (out_ptr == out_end)
>> -					goto out;
>> -				*out_ptr++ = '%';
>> +				if (cn->used == cn->size)
>> +					if (expand_corename(cn))
>> +						goto out_fail;
>> +
>> +				out_ptr = cn->corename + cn->used;
>> +				*out_ptr = '%';
>> +				cn->used++;
>
>
>> +	out_ptr = cn->corename + cn->used;
>> +	if (cn->used == cn->size)
>> +		if (expand_corename(cn))
>> +			goto out_fail;
>> +
>> +	out_ptr = cn->corename + cn->used;
>>   	*out_ptr = 0;
>
> Quite a bit of code duplication there.  A little helper function which
> adds a single char to the output would tidy that up.
Yep, this would be much more cleaner ;-)
>
> However I think that if the % and %% handers are converted to call
> cn_printf() then the output is always null-terninated and the third
> hunk of code above simply becomes unneeded?
You are absolutely right ;-)
>
> Something like this, although I didn't try very hard.  Just a
> suggestion to work with ;)
>
Yep,  we just need change a little on your patch

	- err = cn_printf(cn, "%%");
	+ err = cn_printf(cn, "%");

Do you need me to resend a v4 patch?

Re: [RFC PATCH v3] core_pattern: fix long parameters was truncated by core_pattern handler

[Andrew Morton]

On Tue, 24 Aug 2010 14:18:32 +0800 Xiaotian Feng <dfeng@redhat.com> wrote:

> >
> > Something like this, although I didn't try very hard.  Just a
> > suggestion to work with ;)
> >
> Yep,  we just need change a little on your patch
> 
> 	- err = cn_printf(cn, "%%");
> 	+ err = cn_printf(cn, "%");

whoa.  Closer than I usually achieve.

> Do you need me to resend a v4 patch?

Yes please - something which was nicely tested ;)

[PATCH v4] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
        argc[10]=<12807486>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE. So if the last parameter is %
specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
this will write out of corename array.

Changes since v3:
make handling of single char also uses cn_printf, suggested by Andrew Morton.

Changes since v2:
Introduced generic function cn_printf and make format_corename remember the time
has been expanded, suggested by Olg Nesterov and Neil Horman.

Changes since v1:
This patch allocates corename at runtime, if the replace doesn't have enough
memory, expand the corename dynamically, suggested by Neil Horman.

I've tested with some core_pattern strings, it works fine now.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |  158 +++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 97 insertions(+), 61 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 2d94552..0151923 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -65,6 +65,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
 int suid_dumpable = 0;
 
+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 LIST_HEAD(formats);
@@ -1440,127 +1446,149 @@ void set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static int expand_corename(struct core_name *cn)
+{
+	char *old_corename = cn->corename;
+
+	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);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int cn_printf(struct core_name *cn, const char *fmt, ...)
+{
+	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))
+		goto out_printf;
+
+	ret = expand_corename(cn);
+	if (ret)
+		goto expand_fail;
+
+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;
+
+expand_fail:
+	return ret;
+}
+
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(char *corename, long signr)
+static int format_corename(struct core_name *cn, long signr)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
-	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
-	int rc;
 	int pid_in_pattern = 0;
+	int err = 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;
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (out_ptr == out_end)
-				goto out;
-			*out_ptr++ = *pat_ptr++;
+			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
 			switch (*++pat_ptr) {
+			/* single % at the end, drop that */
 			case 0:
+				err = cn_printf(cn, "%c", '\0');
+				if (err)
+					break;
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (out_ptr == out_end)
-					goto out;
-				*out_ptr++ = '%';
+				err = cn_printf(cn, "%c", '%');
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", task_tgid_vnr(current));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d",
+					      task_tgid_vnr(current));
 				break;
 			/* uid */
 			case 'u':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->uid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d", cred->uid);
 				break;
 			/* gid */
 			case 'g':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->gid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d", cred->gid);
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%ld", signr);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%ld", signr);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", tv.tv_sec);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%lu", tv.tv_sec);
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", utsname()->nodename);
+				err = cn_printf(cn, "%s",
+					      utsname()->nodename);
 				up_read(&uts_sem);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
 				break;
 			/* executable */
 			case 'e':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", current->comm);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%s", current->comm);
 				break;
 			/* core limit size */
 			case 'c':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", rlimit(RLIMIT_CORE));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%lu",
+					      rlimit(RLIMIT_CORE));
 				break;
 			default:
 				break;
 			}
 			++pat_ptr;
 		}
+
+		if (err)
+			return err;
 	}
+
 	/* Backward compatibility with core_uses_pid:
 	 *
 	 * If core_pattern does not include a %p (as is the default)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
 	if (!ispipe && !pid_in_pattern && core_uses_pid) {
-		rc = snprintf(out_ptr, out_end - out_ptr,
-			      ".%d", task_tgid_vnr(current));
-		if (rc > out_end - out_ptr)
-			goto out;
-		out_ptr += rc;
+		err = cn_printf(cn, ".%d", task_tgid_vnr(current));
+		if (err)
+			return err;
 	}
 out:
-	*out_ptr = 0;
 	return ispipe;
 }
 
@@ -1837,7 +1865,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	struct core_name cn;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
@@ -1892,7 +1920,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	ispipe = format_corename(corename, signr);
+	ispipe = format_corename(&cn, signr);
+
+	if (ispipe == -ENOMEM) {
+		printk(KERN_WARNING "format_corename failed\n");
+		printk(KERN_WARNING "Aborting core\n");
+		goto fail_corename;
+	}
 
  	if (ispipe) {
 		int dump_count;
@@ -1929,7 +1963,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, corename+1, NULL);
+		helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
@@ -1942,7 +1976,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		argv_free(helper_argv);
 		if (retval) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
-			       corename);
+			       cn.corename);
 			goto close_fail;
  		}
 	} else {
@@ -1951,7 +1985,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		if (cprm.limit < binfmt->min_coredump)
 			goto fail_unlock;
 
-		cprm.file = filp_open(corename,
+		cprm.file = filp_open(cn.corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
 		if (IS_ERR(cprm.file))
@@ -1993,6 +2027,8 @@ fail_dropcount:
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(cn.corename);
+fail_corename:
 	coredump_finish(mm);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.7.2.1

Re: [PATCH v4] core_pattern: fix long parameters was truncated by core_pattern handler

[Andrew Morton]

On Tue, 24 Aug 2010 17:42:46 +0800
Xiaotian Feng <dfeng@redhat.com> wrote:

> We met a parameter truncated issue, consider following:
> > > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
> /proc/sys/kernel/core_pattern
> 
> This is okay because the strings is less than CORENAME_MAX_SIZE.
> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
> after we run core_pattern_pipe_test in man page, we found last
> parameter was truncated like below:
>         argc[10]=<12807486>
> 
> The root cause is core_pattern allows % specifiers, which need to be
> replaced during parse time, but the replace may expand the strings
> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
> this will write out of corename array.
> 
> Changes since v3:
> make handling of single char also uses cn_printf, suggested by Andrew Morton.
> 
> Changes since v2:
> Introduced generic function cn_printf and make format_corename remember the time
> has been expanded, suggested by Olg Nesterov and Neil Horman.
> 
> Changes since v1:
> This patch allocates corename at runtime, if the replace doesn't have enough
> memory, expand the corename dynamically, suggested by Neil Horman.
> 
> I've tested with some core_pattern strings, it works fine now.

cool, thanks.

> 
> ...
>
> -static int format_corename(char *corename, long signr)
> +static int format_corename(struct core_name *cn, long signr)
>  {
>  	const struct cred *cred = current_cred();
>  	const char *pat_ptr = core_pattern;
>  	int ispipe = (*pat_ptr == '|');
> -	char *out_ptr = corename;
> -	char *const out_end = corename + CORENAME_MAX_SIZE;
> -	int rc;
>  	int pid_in_pattern = 0;
> +	int err = 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;
>  
>  	/* Repeat as long as we have more pattern to process and more output
>  	   space */
>  	while (*pat_ptr) {
>  		if (*pat_ptr != '%') {
> -			if (out_ptr == out_end)
> -				goto out;
> -			*out_ptr++ = *pat_ptr++;
> +			err = cn_printf(cn, "%c", *pat_ptr++);
>  		} else {
>  			switch (*++pat_ptr) {
> +			/* single % at the end, drop that */
>  			case 0:
> +				err = cn_printf(cn, "%c", '\0');

Confused.  Doesn't this bit just add another \0 to the end of an
already-null-terminated string?  And then make cn->used get out of sync
with strlen(cn->corename)?

Re: [PATCH v4] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

On 08/25/2010 06:47 AM, Andrew Morton wrote:
> On Tue, 24 Aug 2010 17:42:46 +0800
> Xiaotian Feng<dfeng@redhat.com>  wrote:
>
>> We met a parameter truncated issue, consider following:
>>>> echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
>> %s %c %p %u %g 11 12345678901234567890123456789012345678 %t">  \
>> /proc/sys/kernel/core_pattern
>>
>> This is okay because the strings is less than CORENAME_MAX_SIZE.
>> "cat /proc/sys/kernel/core_pattern" shows the whole string. but
>> after we run core_pattern_pipe_test in man page, we found last
>> parameter was truncated like below:
>>          argc[10]=<12807486>
>>
>> The root cause is core_pattern allows % specifiers, which need to be
>> replaced during parse time, but the replace may expand the strings
>> to larger than CORENAME_MAX_SIZE. So if the last parameter is %
>> specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
>> this will write out of corename array.
>>
>> Changes since v3:
>> make handling of single char also uses cn_printf, suggested by Andrew Morton.
>>
>> Changes since v2:
>> Introduced generic function cn_printf and make format_corename remember the time
>> has been expanded, suggested by Olg Nesterov and Neil Horman.
>>
>> Changes since v1:
>> This patch allocates corename at runtime, if the replace doesn't have enough
>> memory, expand the corename dynamically, suggested by Neil Horman.
>>
>> I've tested with some core_pattern strings, it works fine now.
>
> cool, thanks.
>
>>
>> ...
>>
>> -static int format_corename(char *corename, long signr)
>> +static int format_corename(struct core_name *cn, long signr)
>>   {
>>   	const struct cred *cred = current_cred();
>>   	const char *pat_ptr = core_pattern;
>>   	int ispipe = (*pat_ptr == '|');
>> -	char *out_ptr = corename;
>> -	char *const out_end = corename + CORENAME_MAX_SIZE;
>> -	int rc;
>>   	int pid_in_pattern = 0;
>> +	int err = 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;
>>
>>   	/* Repeat as long as we have more pattern to process and more output
>>   	   space */
>>   	while (*pat_ptr) {
>>   		if (*pat_ptr != '%') {
>> -			if (out_ptr == out_end)
>> -				goto out;
>> -			*out_ptr++ = *pat_ptr++;
>> +			err = cn_printf(cn, "%c", *pat_ptr++);
>>   		} else {
>>   			switch (*++pat_ptr) {
>> +			/* single % at the end, drop that */
>>   			case 0:
>> +				err = cn_printf(cn, "%c", '\0');
>
> Confused.  Doesn't this bit just add another \0 to the end of an
> already-null-terminated string?  And then make cn->used get out of sync
> with strlen(cn->corename)?
>
Good catch, I just realized the return value of vsnprintf is not 
including the trailing '\0', will follow an updated v5 patch. Thanks Andrew.

[PATCH v5] core_pattern: fix long parameters was truncated by core_pattern handler

[Xiaotian Feng]

We met a parameter truncated issue, consider following:
> > echo "|/root/core_pattern_pipe_test %p /usr/libexec/blah-blah-blah \
%s %c %p %u %g 11 12345678901234567890123456789012345678 %t" > \
/proc/sys/kernel/core_pattern

This is okay because the strings is less than CORENAME_MAX_SIZE.
"cat /proc/sys/kernel/core_pattern" shows the whole string. but
after we run core_pattern_pipe_test in man page, we found last
parameter was truncated like below:
        argc[10]=<12807486>

The root cause is core_pattern allows % specifiers, which need to be
replaced during parse time, but the replace may expand the strings
to larger than CORENAME_MAX_SIZE. So if the last parameter is %
specifiers, the replace code is using snprintf(out_ptr, out_end - out_ptr, ...),
this will write out of corename array.

Changes since v4:
cn_printf needs to take trailing '\0' into account. Then we don't need to
copy extra '\0' while parsing pat_ptr.

Changes since v3:
make handling of single char also uses cn_printf, suggested by Andrew Morton.

Changes since v2:
Introduced generic function cn_printf and make format_corename remember the time
has been expanded, suggested by Olg Nesterov and Neil Horman.

Changes since v1:
This patch allocates corename at runtime, if the replace doesn't have enough
memory, expand the corename dynamically, suggested by Neil Horman.

I've tested with some core_pattern strings, it works fine now.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Reviewed-by: Neil Horman <nhorman@tuxdriver.com>
Cc: Roland McGrath <roland@redhat.com>
---
 fs/exec.c |  155 +++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 95 insertions(+), 60 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 2d94552..ca3550c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -65,6 +65,12 @@ char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
 int suid_dumpable = 0;
 
+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 LIST_HEAD(formats);
@@ -1440,127 +1446,148 @@ void set_binfmt(struct linux_binfmt *new)
 
 EXPORT_SYMBOL(set_binfmt);
 
+static int expand_corename(struct core_name *cn)
+{
+	char *old_corename = cn->corename;
+
+	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);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int cn_printf(struct core_name *cn, const char *fmt, ...)
+{
+	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;
+
+	ret = expand_corename(cn);
+	if (ret)
+		goto expand_fail;
+
+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;
+
+expand_fail:
+	return ret;
+}
+
 /* format_corename will inspect the pattern parameter, and output a
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(char *corename, long signr)
+static int format_corename(struct core_name *cn, long signr)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
-	char *out_ptr = corename;
-	char *const out_end = corename + CORENAME_MAX_SIZE;
-	int rc;
 	int pid_in_pattern = 0;
+	int err = 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;
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (out_ptr == out_end)
+			if (*pat_ptr == 0)
 				goto out;
-			*out_ptr++ = *pat_ptr++;
+			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
 			switch (*++pat_ptr) {
+			/* single % at the end, drop that */
 			case 0:
 				goto out;
 			/* Double percent, output one percent */
 			case '%':
-				if (out_ptr == out_end)
-					goto out;
-				*out_ptr++ = '%';
+				err = cn_printf(cn, "%c", '%');
 				break;
 			/* pid */
 			case 'p':
 				pid_in_pattern = 1;
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", task_tgid_vnr(current));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d",
+					      task_tgid_vnr(current));
 				break;
 			/* uid */
 			case 'u':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->uid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d", cred->uid);
 				break;
 			/* gid */
 			case 'g':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%d", cred->gid);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%d", cred->gid);
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%ld", signr);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%ld", signr);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
 				struct timeval tv;
 				do_gettimeofday(&tv);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", tv.tv_sec);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%lu", tv.tv_sec);
 				break;
 			}
 			/* hostname */
 			case 'h':
 				down_read(&uts_sem);
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", utsname()->nodename);
+				err = cn_printf(cn, "%s",
+					      utsname()->nodename);
 				up_read(&uts_sem);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
 				break;
 			/* executable */
 			case 'e':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%s", current->comm);
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%s", current->comm);
 				break;
 			/* core limit size */
 			case 'c':
-				rc = snprintf(out_ptr, out_end - out_ptr,
-					      "%lu", rlimit(RLIMIT_CORE));
-				if (rc > out_end - out_ptr)
-					goto out;
-				out_ptr += rc;
+				err = cn_printf(cn, "%lu",
+					      rlimit(RLIMIT_CORE));
 				break;
 			default:
 				break;
 			}
 			++pat_ptr;
 		}
+
+		if (err)
+			return err;
 	}
+
 	/* Backward compatibility with core_uses_pid:
 	 *
 	 * If core_pattern does not include a %p (as is the default)
 	 * and core_uses_pid is set, then .%pid will be appended to
 	 * the filename. Do not do this for piped commands. */
 	if (!ispipe && !pid_in_pattern && core_uses_pid) {
-		rc = snprintf(out_ptr, out_end - out_ptr,
-			      ".%d", task_tgid_vnr(current));
-		if (rc > out_end - out_ptr)
-			goto out;
-		out_ptr += rc;
+		err = cn_printf(cn, ".%d", task_tgid_vnr(current));
+		if (err)
+			return err;
 	}
 out:
-	*out_ptr = 0;
 	return ispipe;
 }
 
@@ -1837,7 +1864,7 @@ static int umh_pipe_setup(struct subprocess_info *info)
 void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 {
 	struct core_state core_state;
-	char corename[CORENAME_MAX_SIZE + 1];
+	struct core_name cn;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
@@ -1892,7 +1919,13 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	ispipe = format_corename(corename, signr);
+	ispipe = format_corename(&cn, signr);
+
+	if (ispipe == -ENOMEM) {
+		printk(KERN_WARNING "format_corename failed\n");
+		printk(KERN_WARNING "Aborting core\n");
+		goto fail_corename;
+	}
 
  	if (ispipe) {
 		int dump_count;
@@ -1929,7 +1962,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, corename+1, NULL);
+		helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
@@ -1942,7 +1975,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		argv_free(helper_argv);
 		if (retval) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
-			       corename);
+			       cn.corename);
 			goto close_fail;
  		}
 	} else {
@@ -1951,7 +1984,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		if (cprm.limit < binfmt->min_coredump)
 			goto fail_unlock;
 
-		cprm.file = filp_open(corename,
+		cprm.file = filp_open(cn.corename,
 				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
 				 0600);
 		if (IS_ERR(cprm.file))
@@ -1993,6 +2026,8 @@ fail_dropcount:
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(cn.corename);
+fail_corename:
 	coredump_finish(mm);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.7.2.1