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 AEC9CD37499 for ; Thu, 17 Oct 2024 15:47:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DB0996B007B; Thu, 17 Oct 2024 11:47:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D600E6B0082; Thu, 17 Oct 2024 11:47:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C4E4B6B0083; Thu, 17 Oct 2024 11:47:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id A286C6B007B for ; Thu, 17 Oct 2024 11:47:11 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id E6AFDA14CD for ; Thu, 17 Oct 2024 15:46:50 +0000 (UTC) X-FDA: 82683522768.24.D13F22F Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf07.hostedemail.com (Postfix) with ESMTP id 9F92C4000E for ; Thu, 17 Oct 2024 15:46:55 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sQLLNmbR; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf07.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=1729179869; 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=RYI60+V1OjqqbJvzM+AKoig6oxKGbuSd60iiQNm2O4s=; b=ItyC8Mi54JLK0enuGXQqJNpAQeTtvbOgrYXhVNUzGTzIXRpg7Z+DS25SLivu46bnEtdZ8n OZDd2fdCuz7UBk+SqwWQCMe/XJ1ZHo/2iObgFChJjxgThDKXFd+U9hBasFfkvDY7cvi5km VH/v/FGMmOvUpNsSNGw0wh66JpJi35w= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729179869; a=rsa-sha256; cv=none; b=X9cXjIsu8tQRu1mR0lW5wX4cIXuWdCxATddlrJHE1s50CI3Eeo3KWFG+8h0gd5Jk87tITP W4tsXxaDps5yr9rpS54+RstZChl7pW/y9J3GmhSG25vUX8bDdOBBLnqK9sKA733Z8R7zNw /OZ+jZ3NxKxDmob6R537gjSYh5nHjYo= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sQLLNmbR; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf07.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 95C21A44102; Thu, 17 Oct 2024 15:46:59 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 31A02C4CEC3; Thu, 17 Oct 2024 15:47:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729180028; bh=Uvx5j39oQwVjqMRql4ui9TrzD7d3TLD7Bw2r34x+59A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sQLLNmbRBlRc4oyZ+uiiRgwg8u9G3p7xZ/R8kJ2htAWL/8gCd7EeNLXr9DUD6+35f AUNQ2vXxOoyaEmJLfyZ8wQA1kFy4RWHT+vtidmgbPofSdvpPoCT10i3QNfVUkfF8Lr EEAsRgiGcd9HrhakR183XerTwEnUYLN0djK7iQSugnqLj69pdQTx1HiLf4w4Av7sY6 gCPy1vkBaE32D1qa6Xoj6/2rXNYVh+YazQCkwzT07xdWixXAYMlN7pBwav3y2jLgk4 SO6s1lkjpRUz3jhlHs+k6exXJRILxndnjyVx5PqERHTIc/70V5e5g2xUA2WvXSEdLe 400Sv9+GKLdgA== Date: Thu, 17 Oct 2024 08:47:03 -0700 From: Kees Cook To: Tycho Andersen Cc: Zbigniew =?utf-8?Q?J=C4=99drzejewski-Szmek?= , "Eric W. Biederman" , Alexander Viro , Christian Brauner , Jan Kara , Jeff Layton , Chuck Lever , Alexander Aring , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Tycho Andersen , Aleksa Sarai Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm Message-ID: <202410170840.8E974776@keescook> References: <20240924141001.116584-1-tycho@tycho.pizza> <87msjx9ciw.fsf@email.froward.int.ebiederm.org> <202410141403.D8B6671@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 9F92C4000E X-Stat-Signature: y4e3p7ayuzyfnstyx5djjx7ukicr39nr X-Rspam-User: X-HE-Tag: 1729180015-85455 X-HE-Meta: U2FsdGVkX1+YJ2eeojJ7u2FO5B/svRKsQ/TXW+jas/rw+4iPVHaW5ZpglDUGSwKQA32b06UwgJtavI6rHmal04FChuMjHLi9GQ3IC2C+Bl7q9W2cV+N5pdmE8Z3jaTHEoN/wdRKU7DDS1rB7wZuNv0Z94PwcjToFqSaxdYLilcUtmWN8CpdsGI9r4Ps93ptaUt9SNmT7J/6VDR/y5LN6Tt6aJsmthuj5f4bcl9XKIg5ynrfIV4ZWB2n3qceicG3SqAgI4pAcUo4gDE8x+r/qgnHqp2PX6dG0zVpB1IM+xUuEZSCQf2OWg7H3aCULJSldVPdgBUVv+N/baU7XFuYCPg02jbkHCPVMG4h8ZTM6FxIgA3/RZt0DBdMnXQmiHXWI8uMyASplaZ+MecN2M+jiRL47V7aywakXhY0QarQek3bcjft7thDPX4G4KYZwzrX5IZQSGW9FkIP8r+3bemGWZeqc61mAtIc0T32gDj2LHrvKdMZ5us33mQ4LQ3T1VTlacrsWJct6Ms24PsZ69Wff9DM4KOibZEoGV2P6V9nOG59ovNbVhkj0eyh3qbA75LOG2ixNMndLLQzYtgJwxlnvq4CFfLVcvD8qFsWBWBnqbj4Ds776yD68ERWOfo5hAcObLabkOPeFr62JIMoFl3GDwEANLPePw2IKeJf3sSsxJghgUsyKx/E+CTtZxW46GMJeAppgyyhUsdhpZRNi0HqdEHSgiOQy917lNW1HgxSptuyQKC5pX450goUFCmFg813nynEuAO+ntEMcB+lk6j3mpP2AgCD6Xb/r+RfdPZV7O4+SZxAfVGWBnvp/lZL4b1ct6ghuHT5xK41WMJJLOy+CTucGzfnReaot/tc4DBpSrhxZR/kAFG1Q6rX/x43Oc+zCbEPVFmj2X6cBqKVbvFhb50l+V1ijW3HuaWwUyPKyqG9CxF7fNVrVusbV/NRE2wHfW3pZSNvJKggCn8tLLcu lIVmJeZz 1IJAadsHYcm8s5UwbSZTcBhQLsGmrCuH+9dR0PuowQGpL5sKwd5c589M9j2tnf7XVQZRhbHOOcJuW7KcRaBEdePXNS/xYkuDBAT/eqfYzTAI+UY98/r1kkAk4xDt4piv4K6o2YEO3/zHn2tHkgJQk/QDTgvQ73E30yDukSaeT1qMQDwBmDsM+sEzLc4tka3WxziWoLO7jbydjfsIh6uB/vSmZbwWlYlejfpxi15kHw0UmvW6xC1b9evsP6Ci3cxzblFfQFhSMgv6WRJpOBgTwhtu+i4RlbM52pNSOXspAj519YDmwX9wZOJmjBlfDXnzmatV+ZoBmYI19gNc= 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 Thu, Oct 17, 2024 at 08:34:43AM -0600, Tycho Andersen wrote: > On Mon, Oct 14, 2024 at 02:13:32PM -0700, Kees Cook wrote: > > On Wed, Oct 09, 2024 at 08:41:31AM -0600, Tycho Andersen wrote: > > > +static int bprm_add_fixup_comm(struct linux_binprm *bprm, struct user_arg_ptr argv) > > > +{ > > > + const char __user *p = get_user_arg_ptr(argv, 0); > > > + > > > + /* > > > + * In keeping with the logic in do_execveat_common(), we say p == NULL > > > + * => "" for comm. > > > + */ > > > + if (!p) { > > > + bprm->argv0 = kstrdup("", GFP_KERNEL); > > > + return 0; > > > + } > > > + > > > + bprm->argv0 = strndup_user(p, MAX_ARG_STRLEN); > > > + if (bprm->argv0) > > > + return 0; > > > + > > > + return -EFAULT; > > > +} > > > > I'd rather this logic got done in copy_strings() and to avoid duplicating > > a copy for all exec users. I think it should be possible to just do > > this, to find the __user char *: > > > > diff --git a/fs/exec.c b/fs/exec.c > > index 77364806b48d..e12fd706f577 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -642,6 +642,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > > goto out; > > } > > } > > + if (argc == 0) > > + bprm->argv0 = str; > > } > > ret = 0; > > out: > > Isn't str here a __user? We want a kernel string for setting comm, so > I guess kaddr+offset? But that's not mapped any more... Yes, but it'll be valid __user addr in the new process. (IIUC) > > Once we get to begin_new_exec(), only if we need to do the work (fdpath > > set), then we can do the strndup_user() instead of making every exec > > hold a copy regardless of whether it will be needed. > > What happens if that allocation fails? begin_new_exec() says it is the > point of no return, so we would just swallow the exec? Or have > mysteriously inconsistent behavior? If we can't alloc a string in begin_new_exec() we're going to have much later problems, so yeah, I'm fine with it failing there. > I think we could check ->fdpath in the bprm_add_fixup_comm() above, > and only do the allocation when really necessary. I should have done > that in the above version, which would have made the comment about > checking fdpath even somewhat true :) But to keep this more readable, I do like your version below, with some notes. > > Something like the below? > > Tycho > > > > diff --git a/fs/exec.c b/fs/exec.c > index dad402d55681..7ec0bbfbc3c3 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1416,7 +1416,16 @@ int begin_new_exec(struct linux_binprm * bprm) > set_dumpable(current->mm, SUID_DUMP_USER); > > perf_event_exec(); > - __set_task_comm(me, kbasename(bprm->filename), true); > + > + /* > + * If argv0 was set, execveat() made up a path that will > + * probably not be useful to admins running ps or similar. > + * Let's fix it up to be something reasonable. > + */ > + if (bprm->argv0) > + __set_task_comm(me, kbasename(bprm->argv0), true); > + else > + __set_task_comm(me, kbasename(bprm->filename), true); > > /* An exec changes our domain. We are no longer part of the thread > group */ > @@ -1566,9 +1575,36 @@ static void free_bprm(struct linux_binprm *bprm) > if (bprm->interp != bprm->filename) > kfree(bprm->interp); > kfree(bprm->fdpath); > + kfree(bprm->argv0); > kfree(bprm); > } > > +static int bprm_add_fixup_comm(struct linux_binprm *bprm, struct user_arg_ptr argv) > +{ > + const char __user *p = get_user_arg_ptr(argv, 0); To keep this const but not call get_user_arg_ptr() before the fdpath check, how about externalizing it. See further below... > + > + /* > + * If this isn't an execveat(), we don't need to fix up the command. > + */ > + if (!bprm->fdpath) > + return 0; > + > + /* > + * In keeping with the logic in do_execveat_common(), we say p == NULL > + * => "" for comm. > + */ > + if (!p) { > + bprm->argv0 = kstrdup("", GFP_KERNEL); Do we want an empty argv0, though? Shouldn't an empty fall back to fdpath? > + return 0; > + } > + > + bprm->argv0 = strndup_user(p, MAX_ARG_STRLEN); > + if (bprm->argv0) > + return 0; > + > + return -EFAULT; > +} > + > static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int flags) > { > struct linux_binprm *bprm; > @@ -1975,6 +2011,10 @@ static int do_execveat_common(int fd, struct filename *filename, > goto out_ret; > } > > + retval = bprm_add_fixup_comm(bprm, argv); > + if (retval != 0) > + goto out_free; How about: if (unlikely(bprm->fdpath)) { retval = bprm_add_fixup_comm(bprm, argv); if (retval != 0) goto out_free; } with the fdpath removed from bprm_add_fixup_comm()? > + > retval = count(argv, MAX_ARG_STRINGS); > if (retval == 0) > pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n", -- Kees Cook