linux-kernel - Re: [PATCH v2 5/7] stacktrace: introduce snprint_stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20141121153759.c6a502e824207d517dd2f994@linux-foundation.org>
Date:	Fri, 21 Nov 2014 15:37:59 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Joonsoo Kim <iamjoonsoo.kim@....com>
Cc:	Mel Gorman <mgorman@...e.de>, Johannes Weiner <hannes@...xchg.org>,
	Minchan Kim <minchan@...nel.org>, Dave Hansen <dave@...1.net>,
	Michal Nazarewicz <mina86@...a86.com>,
	Jungsoo Son <jungsoo.son@....com>,
	Ingo Molnar <mingo@...hat.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/7] stacktrace: introduce snprint_stack_trace for
 buffer output

On Fri, 21 Nov 2014 17:14:04 +0900 Joonsoo Kim <iamjoonsoo.kim@....com> wrote:

> Current stacktrace only have the function for console output.
> page_owner that will be introduced in following patch needs to print
> the output of stacktrace into the buffer for our own output format
> so so new function, snprint_stack_trace(), is needed.
> 
> ...
>
> --- a/include/linux/stacktrace.h
> +++ b/include/linux/stacktrace.h
> @@ -20,6 +20,8 @@ extern void save_stack_trace_tsk(struct task_struct *tsk,
>  				struct stack_trace *trace);
>  
>  extern void print_stack_trace(struct stack_trace *trace, int spaces);
> +extern int  snprint_stack_trace(char *buf, int buf_len,
> +				struct stack_trace *trace, int spaces);
>  
>  #ifdef CONFIG_USER_STACKTRACE_SUPPORT
>  extern void save_stack_trace_user(struct stack_trace *trace);
> @@ -32,6 +34,7 @@ extern void save_stack_trace_user(struct stack_trace *trace);
>  # define save_stack_trace_tsk(tsk, trace)		do { } while (0)
>  # define save_stack_trace_user(trace)			do { } while (0)
>  # define print_stack_trace(trace, spaces)		do { } while (0)
> +# define snprint_stack_trace(buf, len, trace, spaces)	do { } while (0)

Doing this with macros instead of C functions is pretty crappy - it
defeats typechecking and can lead to unused-var warnings when the
feature is disabled.

Fixing this might not be practical if struct stack_trace isn't
available, dunno.

> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -25,6 +25,30 @@ void print_stack_trace(struct stack_trace *trace, int spaces)
>  }
>  EXPORT_SYMBOL_GPL(print_stack_trace);
>  
> +int snprint_stack_trace(char *buf, int buf_len, struct stack_trace *trace,
> +			int spaces)
> +{
> +	int i, printed;
> +	unsigned long ip;
> +	int ret = 0;
> +
> +	if (WARN_ON(!trace->entries))
> +		return 0;
> +
> +	for (i = 0; i < trace->nr_entries && buf_len; i++) {
> +		ip = trace->entries[i];
> +		printed = snprintf(buf, buf_len, "%*c[<%p>] %pS\n",
> +				1 + spaces, ' ', (void *) ip, (void *) ip);
> +
> +		buf_len -= printed;
> +		ret += printed;
> +		buf += printed;
> +	}
> +
> +	return ret;
> +}

I'm not liking this much.  The behaviour when the output buffer is too
small is scary.  snprintf() will return "the number of characters which
would be generated for the given input", so local variable `buf_len'
will go negative and we pass a negative int into snprintf()'s `size_t
size'.  snprintf() says "goody, lots and lots of buffer!" and your
machine crashes.

buf_len should be a size_t and snprint_stack_trace() will need to be
changed to handle this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/