linux-kernel - Re: [PATCH] string: Improve the generic strlcpy() implementation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <5625194F.30408@ezchip.com>
Date:	Mon, 19 Oct 2015 12:24:47 -0400
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Ingo Molnar <mingo@...nel.org>
CC:	Linus Torvalds <torvalds@...ux-foundation.org>,
	open list <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] string: Improve the generic strlcpy() implementation

On 10/19/2015 08:42 AM, Rasmus Villemoes wrote:
> On Mon, Oct 05 2015, Ingo Molnar <mingo@...nel.org> wrote:
>> Interesting. I noticed that strscpy() says this in its comments:
>>
>>   * In addition, the implementation is robust to the string changing out
>>   * from underneath it, unlike the current strlcpy() implementation.
> Apologies for beating a dead horse, but:
>
> 		c = *(unsigned long *)(src+res);
> 		if (has_zero(c, &data, &constants)) {
> 			data = prep_zero_mask(c, data, &constants);
> 			data = create_zero_mask(data);
> 			*(unsigned long *)(dest+res) = c & zero_bytemask(data);
> 			return res + find_zero(data);
> 		}
> 		*(unsigned long *)(dest+res) = c;
>
> I wonder whether an insane compiler might actually reload c before
> storing to dest+res, so that we'd have exactly the same problem of
> embedded nul characters?

Using READ_ONCE() on x86_64 with gcc 4.8 adds a bit of
overhead; for some reason the compiler converts some
mov instructions to lea+mov, unnecessarily as far as I can see.
And looking at older compilers, gcc 4.4 substantially
pessimizes the output with READ_ONCE, though perhaps we
don't care about compilers that old.

Still, your argument is certainly plausible in terms of
actually guaranteeing the semantics we are claiming rather
than leaving it to the compiler to Do The Right Thing.
Unsurprisingly, the current code is causing the compiler
to do the right thing, but of course past performance is no
guarantee of future results, as they say.

So I'm kind of on the fence, maybe leaning slightly
towards thinking the READ_ONCE semantics is worth it.
Maybe someone with a tip gcc can see if the performance
difference has been fixed there?

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

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