linux-kernel - Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

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]

Date:	Thu, 26 Nov 2015 22:31:43 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:
>> If the string corresponding to a %s specifier can change under us, we
>> might end up copying a \0 byte to the output buffer. There might be
>> callers who expect the output buffer to contain a genuine C string
>> whose length is exactly the snprintf return value (assuming truncation
>> hasn't happened or has been checked for).
>>
>> We can avoid this by only passing over the source string once,
>> stopping the first time we meet a nul byte (or when we reach the given
>> precision), and then letting widen_string() handle left/right space
>> padding. As a small bonus, this code reuse also makes the generated
>> code slightly smaller.
>>
>
> Could it be pair of patches: a) re-use, b) optimize for fuzzy strings?

I'm afraid I have no idea what you mean. The patch is already broken
into three (pull out from dentry(), move helper, do the actual thing to
string()). What's a 'fuzzy string', and what optimization do you think of?

This patch already gives us the bonus of only passing over the source
once instead of twice. (Well, at the expense of a little complicated
logic in case we have a larger field width and not the LEFT flag set,
but these are so extremely rare compared to plain %s that it's not worth
caring about. And in any case, the logic already existed.)

>> Cc: Ingo Molnar <mingo@...nel.org>
>> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
>> ---
>>  lib/vsprintf.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index a021e6380404..63ca52366049 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>>  static noinline_for_stack
>>  char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>>  {
>> -       int len, i;
>> +       int len = 0;
>> +       size_t lim = spec.precision;
>
> Just a nitpick: maybe longer first?

Why?

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