linux-kernel - Re: [patch] add /proc/pid/stack to dump task's stack trace

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: <20081106203520.GD3578@elte.hu>
Date:	Thu, 6 Nov 2008 21:35:20 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Ken Chen <kenchen@...gle.com>
Cc:	Alexey Dobriyan <adobriyan@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [patch] add /proc/pid/stack to dump task's stack trace


* Ken Chen <kenchen@...gle.com> wrote:

> On Thu, Nov 6, 2008 at 11:51 AM, Alexey Dobriyan wrote:
> > Please, name handler proc_pid_stack following current convention.
> > And drop space before casts.
> 
> OK.  handler name changed.  For the space between cast, it looks 
> like there are different styles in the code base, either with or 
> without. I dropped the space since I don't have strong opinion one 
> way or the other.

best way is to run scripts/checkpatch.pl on your patch, that will 
remind you of any potential style issues.

> Also wrap proc_pid_stack() inside CONFIG_STACKTRACE to fix compile 
> time error when config option is not selected.

> +#ifdef CONFIG_STACKTRACE
> +#define MAX_STACK_TRACE_DEPTH	32

How about 64 instead? (it's such a nice round number)

> +static int proc_pid_stack(struct task_struct *task, char *buffer)
> +{
> +	int i, len = 0;
> +	unsigned long *entries;
> +	struct stack_trace trace;
> +
> +	entries = kmalloc(sizeof(*entries) * MAX_STACK_TRACE_DEPTH, GFP_KERNEL);
> +	if (!entries)
> +		goto out;
> +
> +	trace.nr_entries = 0;
> +	trace.max_entries = MAX_STACK_TRACE_DEPTH;
> +	trace.entries = entries;
> +	trace.skip = 0;
> +
> +	read_lock(&tasklist_lock);
> +	save_stack_trace_tsk(task, &trace);
> +	read_unlock(&tasklist_lock);
> +
> +	for (i = 0; i < trace.nr_entries; i++) {
> +		len += sprintf(buffer + len, "[<%p>] %pS\n",
> +				(void *)entries[i], (void *)entries[i]);

hm, this looks like a potential buffer overflow - isnt 'buffer' here 
only valid up to the next PAGE_SIZE boundary?

> +	}

> +	kfree(entries);
> +out:
> +	return len;

Not sure about the error path convention here: in the !entries kmalloc 
failure path, shouldnt we return -ENOMEM? Otherwise userspace will get 
zero length and would retry again and again?

Also, please rename 'out:' to 'error:' - to make it clear that it's an 
error path.

	Ingo
--
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/