linux-kernel - Re: [PATCH] perf: Incorrect use of snprintf results in SEGV

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: <20120307010904.GE5656@infradead.org>
Date:	Tue, 6 Mar 2012 22:09:04 -0300
From:	Arnaldo Carvalho de Melo <acme@...hat.com>
To:	Anton Blanchard <anton@...ba.org>
Cc:	paulus@...ba.org, peterz@...radead.org, mingo@...e.hu,
	dsahern@...il.com, fweisbec@...il.com,
	yanmin_zhang@...ux.intel.com, emunson@...bm.net,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf: Incorrect use of snprintf results in SEGV

Em Wed, Mar 07, 2012 at 11:42:49AM +1100, Anton Blanchard escreveu:
> 
> I have a workload where perf top scribbles over the stack and we
> SEGV. What makes it interesting is that an snprintf is causing this.
> 
> The workload is a c++ gem that has method names over 3000 characters
> long, but snprintf is designed to avoid overrunning buffers. So what
> went wrong?
> 
> The problem is we assume snprintf returns the number of characters
> written:
> 
>     ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", self->level);
> ...
>     ret += repsep_snprintf(bf + ret, size - ret, "%s", self->ms.sym->name);
> 
> Unfortunately this is not how snprintf works. snprintf returns the
> number of characters that would have been written if there was enough
> space. In the above case, if the first snprintf returns a value larger
> than size, we pass a negative size into the second snprintf and
> happily scribble over the stack. If you have 3000 character c++
> methods thats a lot of stack to trample.
> 
> This patch fixes repsep_snprintf by clamping the value at size - 1
> which is the maximum snprintf can write before adding the NULL
> terminator.
> 
> I get the sinking feeling that there are a lot of other uses of
> snprintf that have this same bug, we should audit them all.

Indeed, I wonder what kind of crack pipe I was smoking when I read the
snprintf man page.

Or what kind of such pipe the people who designed snprintf were using
:-(

> 
> Signed-off-by: Anton Blanchard <anton@...ba.org>
> Cc: stable@...nel.org
> ---
> 
> Index: linux-build/tools/perf/util/sort.c
> ===================================================================
> --- linux-build.orig/tools/perf/util/sort.c	2012-03-07 10:58:57.502318907 +1100
> +++ linux-build/tools/perf/util/sort.c	2012-03-07 11:00:58.812423271 +1100
> @@ -33,6 +33,9 @@ static int repsep_snprintf(char *bf, siz
>  		}
>  	}
>  	va_end(ap);
> +
> +	if (n >= (int)size)
> +		return size - 1;
>  	return n;
>  }
>  
--
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/