Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2724 bytes --]

Hi Linus,

On Sat, Aug 17, 2024 at 09:26:21AM GMT, Linus Torvalds wrote:
> On Sat, 17 Aug 2024 at 01:48, Alejandro Colomar <alx@kernel.org> wrote:
> >
> > I would compact the above to:
> >
> >         len = strlen(s);
> >         buf = kmalloc_track_caller(len + 1, gfp);
> >         if (buf)
> >                 strcpy(mempcpy(buf, s, len), "");
> 
> No, we're not doing this kind of horror.

Ok.

> If _FORTIFY_SOURCE has problems with a simple "memcpy and add NUL",
> then _FORTIFY_SOURCE needs to be fixed.

_FORTIFY_SOURCE works (AFAIK) by replacing the usual string calls by
oneis that do some extra work to learn the real size of the buffers.
This means that for _FORTIFY_SOURCE to work, you need to actually call a
function.  Since the "add NUL" is not done in a function call, it's
unprotected (except that sanitizers may protect it via other means).
Here's the fortified version of strcpy(3) in the kernel:

	$ grepc -h -B15 strcpy ./include/linux/fortify-string.h
	/**
	 * strcpy - Copy a string into another string buffer
	 *
	 * @p: pointer to destination of copy
	 * @q: pointer to NUL-terminated source string to copy
	 *
	 * Do not use this function. While FORTIFY_SOURCE tries to avoid
	 * overflows, this is only possible when the sizes of @q and @p are
	 * known to the compiler. Prefer strscpy(), though note its different
	 * return values for detecting truncation.
	 *
	 * Returns @p.
	 *
	 */
	/* Defined after fortified strlen to reuse it. */
	__FORTIFY_INLINE __diagnose_as(__builtin_strcpy, 1, 2)
	char *strcpy(char * const POS p, const char * const POS q)
	{
		const size_t p_size = __member_size(p);
		const size_t q_size = __member_size(q);
		size_t size;

		/* If neither buffer size is known, immediately give up. */
		if (__builtin_constant_p(p_size) &&
		    __builtin_constant_p(q_size) &&
		    p_size == SIZE_MAX && q_size == SIZE_MAX)
			return __underlying_strcpy(p, q);
		size = strlen(q) + 1;
		/* Compile-time check for const size overflow. */
		if (__compiletime_lessthan(p_size, size))
			__write_overflow();
		/* Run-time check for dynamic size overflow. */
		if (p_size < size)
			fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p_size, size, p);
		__underlying_memcpy(p, q, size);
		return p;
	}

> We don't replace a "buf[len] = 0" with strcpy(,""). Yes, compilers may
> simplify it, but dammit, it's an unreadable incomprehensible mess to
> humans, and humans still matter a LOT more.

I understand.  While I don't consider it unreadable anymore (I guess I
got used to it), it felt strange at first.

> 
>                 Linus

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]