[<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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
