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 A6E1FC25B74 for ; Sun, 2 Jun 2024 03:52:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CDDAE6B009E; Sat, 1 Jun 2024 23:52:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C8D5E6B00A0; Sat, 1 Jun 2024 23:52:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B54B66B00A1; Sat, 1 Jun 2024 23:52:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 9970B6B009E for ; Sat, 1 Jun 2024 23:52:43 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id DC41040D83 for ; Sun, 2 Jun 2024 03:52:42 +0000 (UTC) X-FDA: 82184576964.07.6C502F0 Received: from out02.mta.xmission.com (out02.mta.xmission.com [166.70.13.232]) by imf29.hostedemail.com (Postfix) with ESMTP id 7E889120009 for ; Sun, 2 Jun 2024 03:52:40 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=none; spf=pass (imf29.hostedemail.com: domain of ebiederm@xmission.com designates 166.70.13.232 as permitted sender) smtp.mailfrom=ebiederm@xmission.com; dmarc=pass (policy=none) header.from=xmission.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717300360; 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; bh=+5ugnlO//IJ5MaYJcWWdw+IcqD2QYHM90pQSS4PFBOA=; b=NI3NLkOtYzWM0mYQ7oB5y/WCmmeNoeqjICKe9E4GqUXeud8e/lMeEx1zN+D5Qk3avHxw9y Yj/1nNi+cFgzOYoe2iCnwlIkthoqsWeop00p4ERxo0yaKaRc/EWj4oFhT66j3ImVO9g72U 7Yz0i/NbPWw/WkrjrnGA2eWZHqRmdB4= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=none; spf=pass (imf29.hostedemail.com: domain of ebiederm@xmission.com designates 166.70.13.232 as permitted sender) smtp.mailfrom=ebiederm@xmission.com; dmarc=pass (policy=none) header.from=xmission.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717300360; a=rsa-sha256; cv=none; b=BIKVuLEskc5tdGh7Nk5c3lcnN+Smfql/A3FoDFJ/0MI4FA3cnk+zB6PDsL0VRsLdZWzv7H /erP+Yaq3DdA1mjUn7II7iUX+QZvEFqGS1YRQ4hBnyrmsSE42bagNP4gMSlif/I18ARTlh Ofv5cpuIBA2j0LGdkoK++zV6viyLIcM= Received: from in02.mta.xmission.com ([166.70.13.52]:36664) by out02.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1sDcGs-00Eawn-4E; Sat, 01 Jun 2024 21:52:38 -0600 Received: from ip68-227-168-167.om.om.cox.net ([68.227.168.167]:34300 helo=email.froward.int.ebiederm.org.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1sDcGq-00F7Wn-Mp; Sat, 01 Jun 2024 21:52:37 -0600 From: "Eric W. Biederman" To: Yafang Shao Cc: torvalds@linux-foundation.org, 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, Alexander Viro , Christian Brauner , Jan Kara , Kees Cook References: <20240602023754.25443-1-laoar.shao@gmail.com> <20240602023754.25443-2-laoar.shao@gmail.com> Date: Sat, 01 Jun 2024 22:51:57 -0500 In-Reply-To: <20240602023754.25443-2-laoar.shao@gmail.com> (Yafang Shao's message of "Sun, 2 Jun 2024 10:37:49 +0800") Message-ID: <87ikysdmsi.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1sDcGq-00F7Wn-Mp;;;mid=<87ikysdmsi.fsf@email.froward.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.168.167;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX1860tjt8nodVMvTZzeRt1UmJFO9w9v1+IM= X-SA-Exim-Connect-IP: 68.227.168.167 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 1/6] fs/exec: Drop task_lock() inside __get_task_comm() X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 7E889120009 X-Stat-Signature: ty8ifxb4fbg9jsn75dnfgfj75rkwsuuh X-HE-Tag: 1717300360-994769 X-HE-Meta: U2FsdGVkX18XchwdKZr47Tt2s/+KsCitF5AuEkkgxHdbzJP6bHqhHQGozW2hk3uMPs/0le1MihnSRLnIoaLh7mvDW7B+Bujm9MC/+asdJRKZhfKetkk6IYXkpYy80xBmugqRk0eHxVdGhLAuTAnwo7QXGyXNmVsE2O+XFdzAoLN764F0WNeDw2yM0zEl5UaY8Tokg1x6v8zb+XIqyaClNrOgGp3fL8IIEnVTtnKG2KfjkGOq35CWjiTgdWDpiWheBEuwB2tin8y+b1wt+0QEyMB8prE9GkCWKSo9ZMMzvqfis3eAt25i5mSvmlsoAwKpwYPyHiOSjMEpCwslvDh+JLeNn57DikH0jWXkPDn+RM7kxAdc/wSNY+MIapbQymitDvxqi1iDD1fUzQ42t0RL/vVPKEs0ZbmRB39oABdl92FC6deVfUpWDWgtXA+1d7ot4jGPkSFp6Bx4PQRYdGz0AWBEC+jrmmxyjCJNOewzrydIWy6TC8OIN5ngpX/V17u0kZHrdBQm9EOu3kXpgJ71k2ufdpYusJOFwoVPIPa21slChmczrVPm0uWvI6KzecBy90Q9NHWsOjuHISCaBjNrpQ1YR9p+QZCjBL+2un9iHHl2v6AmrS1miMfFXG+Y51JdiJFRkU5DbzQLdrK5gvpBwqePE5/kd6bRzez601StgWYNPCs0gklVkfNBD+egznXzJdGh0RozeJV+D9Ho4nlrPXIC0ROr3jOYjilEPJLPMylmZbGmV7Za+bl/nOfG1qqu+gVfSxF3AcYL1P2+ybzX9bmvzi1na3tpFZsqcbGDsKIqIoRv7ZKIPNxVgnWDWY5bcFqLF7ybl923oxTleD+XSHGrE5yOf0hbizB2TysZN04hRIh4fmOJ+Lu/vsO5vao3cHCjhKZyrLVsTMj+xq0/OUNdptT6MvnyXsO8f1BnWta+6KptKs0ewDnhpzZ6Ba7Rwi1PxJ/KxkNqh50134d 7xMK67MN 5rrTn2SCResubhCPuv5HHLZ8h74seGXGTTM5om2bOYKduisITi2VIlmbhwHJ1EAVIHQV8JykNjPNuVwf/s8nzBYYwlBuwE/3rqKZ82zTOjVeHUe+5fZYn4y13Adx4+2+CfawTNJKNcd8rwYw0zXirJQLbFrJN/GoYq9NPBL1WQ8AhIcCXdnvkl/GTNYmuFCjy6nJ3lTcyJNttrkLuEdbBQf4L8sFmIdSKiadErjzFzHg+mE+8tguatPemkuOiA0BZX3jS4sGlO5Lj+9AyogpWtsvVq76uQ8fq6uN7HA4AHDNn9Lv3jOnHCXDoipjvDX9MxJYnyCWWMQIASSFRZ+I/KqDJGnFScdOyR03wvLJwxfgwLmjBHIrYjUGTnIQKRL2aBg0+35GcRC0yLZvbTkGFCUkU8zaHvwSlAziYDyq67iCUBNmV/5k1IMOo4ILP8UO5EM2p0R70NIlhy6rHZ7JrpaQh8A== 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: Yafang Shao writes: > Quoted from Linus [0]: > > Since user space can randomly change their names anyway, using locking > was always wrong for readers (for writers it probably does make sense > to have some lock - although practically speaking nobody cares there > either, but at least for a writer some kind of race could have > long-term mixed results Ugh. Ick. This code is buggy. I won't argue that Linus is wrong, about removing the task_lock. Unfortunately strscpy_pad does not work properly with the task_lock removed, and buf_size larger that TASK_COMM_LEN. There is a race that will allow reading past the end of tsk->comm, if we read while tsk->common is being updated. So __get_task_comm needs to look something like: char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) { size_t len = buf_size; if (len > TASK_COMM_LEN) len = TASK_COMM_LEN; memcpy(buf, tsk->comm, len); buf[len -1] = '\0'; return buf; } What shows up in buf past the '\0' is not guaranteed in the above version but I would be surprised if anyone cares. If people do care the code can do something like: char *last = strchr(buf); memset(last, '\0', buf_size - (last - buf)); To zero everything in the buffer past the first '\0' byte. Eric > Suggested-by: Linus Torvalds > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] > Signed-off-by: Yafang Shao > Cc: Alexander Viro > Cc: Christian Brauner > Cc: Jan Kara > Cc: Eric Biederman > Cc: Kees Cook > --- > fs/exec.c | 7 +++++-- > include/linux/sched.h | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index b3c40fbb325f..b43992d35a8a 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1227,12 +1227,15 @@ static int unshare_sighand(struct task_struct *me) > return 0; > } > > +/* > + * User space can randomly change their names anyway, so locking for readers > + * doesn't make sense. For writers, locking is probably necessary, as a race > + * condition could lead to long-term mixed results. > + */ > char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) > { > - task_lock(tsk); > /* Always NUL terminated and zero-padded */ > strscpy_pad(buf, tsk->comm, buf_size); > - task_unlock(tsk); > return buf; > } > EXPORT_SYMBOL_GPL(__get_task_comm); > diff --git a/include/linux/sched.h b/include/linux/sched.h > index c75fd46506df..56a927393a38 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1083,7 +1083,7 @@ struct task_struct { > * > * - normally initialized setup_new_exec() > * - access it with [gs]et_task_comm() > - * - lock it with task_lock() > + * - lock it with task_lock() for writing > */ > char comm[TASK_COMM_LEN];