From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AFAEACF6496 for ; Sat, 28 Sep 2024 21:17:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3D3B08D0008; Sat, 28 Sep 2024 17:17:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 35C488D0001; Sat, 28 Sep 2024 17:17:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1D4D18D0008; Sat, 28 Sep 2024 17:17:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id ED3528D0001 for ; Sat, 28 Sep 2024 17:17:33 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 8343912148E for ; Sat, 28 Sep 2024 21:17:33 +0000 (UTC) X-FDA: 82615408386.06.34B8B25 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf04.hostedemail.com (Postfix) with ESMTP id EA13540003 for ; Sat, 28 Sep 2024 21:17:31 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=RKee8TD0; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf04.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1727558214; a=rsa-sha256; cv=none; b=skPgM+ftGGFcaPvWyAKbTnJjt7nHkgWQbHa1Md9dA/QeKvCZUDiUvWxp3M8QjWkoFabH3b 3OOIRSU/Ln4tLnKyCGeZW1kYx7dVog4kGJps1SmLypNEh81HAMV3Go+Ep+nmKysA0Ds2h3 VOEp+jjPB/dtSd04UFvbmV5KbbHi4OI= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=RKee8TD0; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf04.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1727558214; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=oiWjfiHX7sOERfxiVImXcu5YpJyr4/5Sz3UXmTgIZ8Q=; b=qZaExapR68u9THDraad5er7RUxzqIlEh5NxvWr5JALhQfVUyBOWMQLFDIW8zvKcwmCoXMV iOtOumCVuIikylxnF3T2VirrgaBz2CNWV065w70s9vNSwXu3lt/b8I2vKg0PYKXKsEhiwr DMT7Uk44V6ay2zHUnCVoX1aZ03Pm0Fs= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id DBD91A40117; Sat, 28 Sep 2024 21:17:22 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9DAFFC4CEC3; Sat, 28 Sep 2024 21:17:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1727558250; bh=D0r2pMAslanB4R9KKI1cz+nAetpbVmkKyvYYI304mqI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RKee8TD0HOzA1sEOmQy+o1YoprUqIz5Nx9I6rrFKrUGy1HZUkPMmOjKnCl3+9maWg 1GdsNYYM0sAY/FVkiThXRc3afmVOcZcblIxwyo55OD2/2r3pt+VjmF531qy1ec/AF/ nJ4AaI0nancNp+mrRBLDnG3AJJeb4kjXv2FtVJ/VHHglFEDhz9o/mxI/jlnp5X2DlS 5Ct0im2UfESxxYEUowIKaR8/cSYh6y1p9AT6lp4oAwdkIRqBdVlWkO89X7OXf1CR2j eKkhJhuoCnM9eYUO/a3XQ19tYfht2hhlkKf5OKlyJQPedvRhQ73TS2vCTd6BMSc8ZQ cA5757sItzN/g== Date: Sat, 28 Sep 2024 14:17:30 -0700 From: Kees Cook To: Alejandro Colomar Cc: Yafang Shao , akpm@linux-foundation.org, torvalds@linux-foundation.org, justinstitt@google.com, ebiederm@xmission.com, alexei.starovoitov@gmail.com, rostedt@goodmis.org, catalin.marinas@arm.com, penguin-kernel@i-love.sakura.ne.jp, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, audit@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup() Message-ID: <202409281414.487BFDAB@keescook> References: <20240817025624.13157-1-laoar.shao@gmail.com> <20240817025624.13157-6-laoar.shao@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: EA13540003 X-Rspamd-Server: rspam01 X-Stat-Signature: 397sfesedsrsaksoxewhrord9x4dsfsc X-HE-Tag: 1727558251-540906 X-HE-Meta: U2FsdGVkX19y884fgY8BUDq/yaZYJ6f0/Tz9NGRfpDCdQUgryRmJ4o06O5/Uv8hWHLJ1ZzRF4BKMi3hR1Yp08AUBwvFRB3XwVzyGb2XHPJ5YJRKIAoBKuI7Tu3F691E9ULO3WWZZWLZQO1KJfksnRdMfMYIGqp85psaPsb5Pw8u30MAWpg1vrmQnj3EcqrBAJgxJKlDobe76/QTGSeb/VkcTD0LR+ZV6t12FdVAMn6nk+7iV+Zyf6a+3L/4KINMt3BKlVf1GQOfql90n0eAninFqp8BViUyK+kAj0E8ABHx7V5zE/wzG1SlTioBHUVzKfsFLFq56OGCLclyTKx9MSy5413Ee0Bk1pzRCAwQKf/lvoAOE2fek4gsFAQNWDGFqfm3cNnezUzjwm3hhxYpomo22PehwrvADblLF6895dHqLvfB2NbGY2PaLWZsqnZnOj8tA0PVbN+RRBrK6fADaBR67lwKxD9aOeZ52jdoN0Qv8U8cpkD59nZXXRrPRVqCoYLjm85/uUeo6l3y/DyACnRuH4g2STmr8kQz/o81ZkGqtlKC3J6InGKyOPQpejR16992jE/XqPK4CuZW3muYcXqbrt164Lb38yiLNBHqvtJsz6RVBnXX/0mQDyn9CrYp1FQzqCCQESkrnOaYmKSK7/ZQFqzynpfXo10FMDq8qJrt6RaiYEVZawwzgsYDJlYnYI2gnxdoGfSJ5cDp+t33XOCQpPJOf6rCpzGKtotsvOo5BexeZWL9h8qrLKAuoDKNCajWXm7CV7jGPifittViDLm9E0xQQVharUIYela9EZ5pSQx0ljc8ERh9sQqhYgexzG06uz4dm4Goz4YLXAja8EEfFD4v5sUjH52g2Gv9FEKQZa7ezoqS6VgiDsGzp6dTR1eVFGiZdG9HhLU1aDW3Z1kEjqMbeOWJIWlzxFatZwORnHZkZFROCB15dhpu5gw4rnUrw14pgIegwNqsDroZ DSOBQw0J FA4WCW7Gfv0knGbit2ySXbM5FmR4qOWaf7GHTigp6u//xJAkTCL19yhCQbTHvivX3AjwoqqoTrKocfHvm3q49ImI3hshLq8P8tqk/u9UZi/qdfPN3HkNvtUiD/SZIgoDNXEuXc/rN0jzKOa0rwTaPhyjUBvSx+RcUUJrnTjN3rjhPMCvAfaLwgGIczDv/asq0tMSEo4iADR5pSj4LAwElxashnABMivM8bbN4UeOJZLc2/EfWtAP/wfklPu1rpz8PK8Wli/UUgSURsJmrjKDRWjzjnOIc/usQZXT02lzMFUnLaIXO4AhEmQoIOfJcMPMvEGrV0ituguQA3/4AkY8NY/X7m6JhOoNN4nb86fVSMDMeElrTNyiZMftXR+SP+EEudB2RsiDRUJNSzKFCB+0iTx7LYg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Sat, Aug 17, 2024 at 10:48:15AM +0200, Alejandro Colomar wrote: > Hi Yafang, > > On Sat, Aug 17, 2024 at 10:56:21AM GMT, Yafang Shao wrote: > > In kstrdup(), it is critical to ensure that the dest string is always > > NUL-terminated. However, potential race condidtion can occur between a > > writer and a reader. > > > > Consider the following scenario involving task->comm: > > > > reader writer > > > > len = strlen(s) + 1; > > strlcpy(tsk->comm, buf, sizeof(tsk->comm)); > > memcpy(buf, s, len); > > > > In this case, there is a race condition between the reader and the > > writer. The reader calculate the length of the string `s` based on the > > old value of task->comm. However, during the memcpy(), the string `s` > > might be updated by the writer to a new value of task->comm. > > > > If the new task->comm is larger than the old one, the `buf` might not be > > NUL-terminated. This can lead to undefined behavior and potential > > security vulnerabilities. > > > > Let's fix it by explicitly adding a NUL-terminator. > > > > Signed-off-by: Yafang Shao > > Cc: Andrew Morton > > --- > > mm/util.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/mm/util.c b/mm/util.c > > index 983baf2bd675..4542d8a800d9 100644 > > --- a/mm/util.c > > +++ b/mm/util.c > > @@ -62,8 +62,14 @@ char *kstrdup(const char *s, gfp_t gfp) > > > > len = strlen(s) + 1; > > buf = kmalloc_track_caller(len, gfp); > > - if (buf) > > + if (buf) { > > memcpy(buf, s, len); > > + /* During memcpy(), the string might be updated to a new value, > > + * which could be longer than the string when strlen() is > > + * called. Therefore, we need to add a null termimator. > > + */ > > + buf[len - 1] = '\0'; > > + } > > I would compact the above to: > > len = strlen(s); > buf = kmalloc_track_caller(len + 1, gfp); > if (buf) > strcpy(mempcpy(buf, s, len), ""); > > It allows _FORTIFY_SOURCE to track the copy of the NUL, and also uses > less screen. It also has less moving parts. (You'd need to write a > mempcpy() for the kernel, but that's as easy as the following:) > > #define mempcpy(d, s, n) (memcpy(d, s, n) + n) > > In shadow utils, I did a global replacement of all buf[...] = '\0'; by > strcpy(..., "");. It ends up being optimized by the compiler to the > same code (at least in the experiments I did). Just to repeat what's already been said: no, please, don't complicate this with yet more wrappers. And I really don't want to add more str/mem variants -- we're working really hard to _remove_ them. :P -Kees -- Kees Cook