linux-kernel - Re: [PATCH V2 1/2] string: Add stracpy and stracpy

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: <24bb53c57767c1c2a8f266c305a670f7@sk2.org>
Date:   Thu, 26 Sep 2019 10:25:37 +0200
From:   Stephen Kitt <steve@....org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Joe Perches <joe@...ches.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Nitin Gote <nitin.r.gote@...el.com>, jannh@...gle.com,
        kernel-hardening@...ts.openwall.com, Takashi Iwai <tiwai@...e.com>,
        Clemens Ladisch <clemens@...isch.de>,
        alsa-devel@...a-project.org
Subject: Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

Le 26/09/2019 09:29, Rasmus Villemoes a écrit :
> On 26/09/2019 02.01, Stephen Kitt wrote:
>> Le 25/09/2019 23:50, Andrew Morton a écrit :
>>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@...ches.com> 
>>> wrote:
>>> 
>>>> Several uses of strlcpy and strscpy have had defects because the
>>>> last argument of each function is misused or typoed.
>>>> 
>>>> Add macro mechanisms to avoid this defect.
>>>> 
>>>> stracpy (copy a string to a string array) must have a string
>>>> array as the first argument (dest) and uses sizeof(dest) as the
>>>> count of bytes to copy.
>>>> 
>>>> These mechanisms verify that the dest argument is an array of
>>>> char or other compatible types like u8 or s8 or equivalent.
>>>> 
>>>> A BUILD_BUG is emitted when the type of dest is not compatible.
>>>> 
>>> 
>>> I'm still reluctant to merge this because we don't have code in -next
>>> which *uses* it.  You did have a patch for that against v1, I 
>>> believe?
>>> Please dust it off and send it along?
>> 
>> Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
>> Here's a different patch which converts some of ALSA's strcpy calls to
>> stracpy:
> 
> Please don't. At least not for the cases where the source is a string
> literal - that just gives worse code generation (because gcc doesn't
> know anything about strscpy or strlcpy), and while a run-time (silent)
> truncation is better than a run-time buffer overflow, wouldn't it be
> even better with a build time error?

Yes, that was the plan once Joe's patch gets merged (if it does), and my
patch was only an example of using stracpy, as a step on the road. I was
intending to follow up with a patch converting stracpy to something like
https://www.openwall.com/lists/kernel-hardening/2019/07/06/14

__FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t 
count)
{
	size_t dest_size = __builtin_object_size(dest, 0);
	size_t src_size = __builtin_object_size(src, 0);
	if (__builtin_constant_p(count) &&
	    __builtin_constant_p(src_size) &&
	    __builtin_constant_p(dest_size) &&
	    src_size <= count &&
	    src_size <= dest_size &&
	    src[src_size - 1] == '\0') {
		strcpy(dest, src);
		return src_size - 1;
	} else {
		return __strscpy(dest, src, count);
	}
}

which, as a macro, would become

#define stracpy(dest, src)						\
({									\
	size_t count = ARRAY_SIZE(dest);				\
	size_t dest_size = __builtin_object_size(dest, 0);		\
	size_t src_size = __builtin_object_size(src, 0);		\
	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
		       __same_type(dest, unsigned char[]) ||		\
		       __same_type(dest, signed char[])));		\
									\
	(__builtin_constant_p(count) &&					\
	 __builtin_constant_p(src_size) &&				\
	 __builtin_constant_p(dest_size) &&				\
	 src_size <= count &&						\
	 src_size <= dest_size &&					\
	 src[src_size - 1] == '\0') ?					\
		(((size_t) strcpy(dest, src)) & 0) + src_size - 1	\
	:								\
		strscpy(dest, src, count);				\
})

and both of these get optimised to movs when copying a constant string
which fits in the target.

I was going at this from the angle of improving the existing APIs and
their resulting code. But I like your approach of failing at compile
time.

Perhaps we could do both ;-).

Regards,

Stephen