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 DD455C3DA7F for ; Mon, 5 Aug 2024 09:26:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 297D56B007B; Mon, 5 Aug 2024 05:26:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 248706B0082; Mon, 5 Aug 2024 05:26:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 10F8F6B0085; Mon, 5 Aug 2024 05:26:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id E78BE6B007B for ; Mon, 5 Aug 2024 05:26:32 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 5D4F280180 for ; Mon, 5 Aug 2024 09:26:32 +0000 (UTC) X-FDA: 82417661424.19.0B98FEB Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf10.hostedemail.com (Postfix) with ESMTP id 8E9CCC0002 for ; Mon, 5 Aug 2024 09:26:28 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=s79evrl8; spf=pass (imf10.hostedemail.com: domain of brauner@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=brauner@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722849959; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=AjIBCLUFuHozamDyZZQClcVEbY5vKToxurIYxmJULoo=; b=tG/XtR63QCLOqrl0Xex77mbKCluOzlAPECoPBeFzGg6cBbZC4zbZSalcSnLGyG6h+/jmmH 0bW7Bz/meVLw45QrK2jbfEvIZm3MQ6SMcVZ5dzoX+AH3upSJV74wgjlgFkK5Q+yYSk/xZA 6wT3W2kNd93+N18rK6RXkDL/MQzu8w4= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=s79evrl8; spf=pass (imf10.hostedemail.com: domain of brauner@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=brauner@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722849959; a=rsa-sha256; cv=none; b=fcDAXISFdB2X2sMN/kioWGLVLf70tL3cR8I4Nqwo2ZBPhx8RPqQfTRbvae7tEsHx9eHy0o qsVMDdyenIaV1jZWaZdYIftNwXgmbla5KVyLeananoKwLOUfQyCBgnF/J+aaj6goojYzQG NMFZcZgiyeF50MgZ3ulxGYXIaZaS7z8= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id ADB24CE09FA; Mon, 5 Aug 2024 09:26:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 73DAFC32782; Mon, 5 Aug 2024 09:26:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722849985; bh=tnpHQw8Llr6uw/Dtux0kJMHq9kupSIzxaNiemENDgwc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=s79evrl8kjfRthVCTcvK2m6/eoaxZ3CL3WB1nA6SKpuaXA2SZe49xN+cEVdb/LhYV ywwjgixEK8xZD/nWp7nKZW+DPY0HQ9xEpRETk8xXzSqby4pqV7WB6QAeKhfHin5h8B BSKkgwk/IBXBy/YeqZfWfXyYkfDSoVodUKH3k+z0szZBIovusHt6fMP1a7YdrHDBlQ BYoTGXAje+pgKvryrp+yLe7qONn5IleGDqdeoOub7T/oo3rcfXN1Wi8FwePbLYe0ce qDe4brVkymcfcX3toKnMw8rl/buz2sZfpWdTow6J/7C7cet4HyktvZYUS8E3KZzt2Q 0vPBBVm8mQeug== Date: Mon, 5 Aug 2024 11:26:19 +0200 From: Christian Brauner To: Mateusz Guzik Cc: Josef Bacik , Wojciech =?utf-8?Q?G=C5=82adysz?= , viro@zeniv.linux.org.uk, jack@suse.cz, ebiederm@xmission.com, kees@kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel/fs: last check for exec credentials on NOEXEC mount Message-ID: <20240805-fehlbesetzung-nilpferd-1ed58783ad4d@brauner> References: <20240801120745.13318-1-wojciech.gladysz@infogain.com> <20240801140739.GA4186762@perftesting> <20240802155859.GB6306@perftesting> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 8E9CCC0002 X-Stat-Signature: g8hmczpfsgmfybbes4oei44rhpheufh3 X-HE-Tag: 1722849988-797447 X-HE-Meta: U2FsdGVkX18qqu2gdlxH5ySmllkflbNsg6dzzzJQTNLgoZYPPUGMjDaxCyxGD3hfuCojzBvX8mvcXBBd4IVzvNTTOoOaB8DhV7OYGkoFWG2jZxOZTv9pulrRUiCL3vgyMEqs8JuDuJQs/DTlJikuyuBHM6lvAFj+9kTUl0IYqMnsQxYyqtE/6aOp+shdZ0Z0u0K8Bd1igpROeDfdlTYqlEf+qnGSb90/RaeeW7AkVatdDnYNlTc3eFKCk6DLo3qBYqKN5OzyHqcCPvuveBRGy+n+IEfhjfAlQsbHgpgJJfFkH18XOzJFfaU0A7Ej93hwSrBge2gW2/WEbkWsW/Gbyzgu/5xLCOTN3f7AThtjuMvQMYQrXi54NZsFOb1vVLlpkRt56FA2XmVjuryYUodqYCV91lC+2/BaIDTdg9bxDEBCXruhpGFRxtfrJce/jigVknJMzYNaeOJlExmEz9EGPJ8cppcWC1hHcz4Px+/8fNljgMj0t5MbLEwJPUPxw3H43pTyxt7VNwK6zISZXp34zDRMUq6vgqFBcOT7cohA7Q1Sb3GmZDNyrMYUtu0cj4qr3VgyEH0i9xSkOScyWZqvsePjB+UFNz6TN+M6KDvvgRqUWeA7prR7AeGZ6t/MvTkIFgnEzxEUqSg5sVGsRkrNhhs/3YKvhFzsTGnKhtaYAieui4v+8mikbNAueTJ1v6xIEZf3aSmlflarLssNw3vvTbbuVNUv8+CWY9nBsgCrr9t4tRHZ0JxBw16Mc8xKurg4I8Lg1/6VCB8tRuZR+FVaNLY4/FKm2ClDXlYtFf9yVHuog4WL9lugxcGMAzNVx0XJKJTJFja8XrxPIQUESdmRstfBfq1f2Lu5n3q928XOcWPVzD3z6tAm2k4EGjVafePPT0oeuiR9wjqnk8/z7hOst+IVzLelfOnlhS84gTZB2aU6C4FG4pjfmAfDUnbd1sfr0tU6+4CsQwO2E7iu80u CvpFy5GN 0QSFArw+prb+TOtDVl8NghC+qwAH8GRQkgeas/BJdauh3BnXmzo3Tx0MzCrUcc65VvXSoOvCYDFR5lE9Xt1EbuIc+YUA/dZwAY2MTnH+NULR6bsU+JOFLLDqt6F+IxCIIwndTp2+HSmbqsK0yd0KjLLv9vmee0HF+Sx1JGoCxSqDIr+pApgrC2DiwUiMnpVfUOPHqMH/ZGMjDal/sifMkFjMBmrmyWgCrSrK2DdifOAfIOMYCGvrGctUyrd9VWFJAs2rBFBUg8+R0mV2eTAhc3iKggnxrpk5USiYu7JKguALOsA+B8HmPW4IXSsGa994KNk/W7VGrbv/9C6z1y7uoUnnPjIZTgY8PYRku 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 03, 2024 at 08:29:17AM GMT, Mateusz Guzik wrote: > On Fri, Aug 2, 2024 at 5:59 PM Josef Bacik wrote: > > > > On Thu, Aug 01, 2024 at 05:15:06PM +0200, Mateusz Guzik wrote: > > > I'm not confident this is particularly valuable, but if it is, it > > > probably should hide behind some debug flags. > > > > I'm still going to disagree here, putting it behind a debug flag means it'll > > never get caught, and it obviously proved valuable because we're discussing this > > particular case. > > > > Is it racy? Yup sure. I think that your solution is the right way to fix it, > > and then we can have a > > > > WARN_ON(!(file->f_mode & FMODE_NO_EXEC_CHECKED)); > > > > or however we choose to flag the file, that way we are no longer racing with the > > mount flags and only validating that a check that should have already occurred > > has in fact occurred. Thanks, > > > > To my understanding the submitter ran into the thing tripping over the > racy check, so this check did not find a real bug elsewhere in this > instance. Mateusz is right. That check is mostly nonsensical. Nothing will protect against mount properties being changed if the caller isn't claiming write access to the mount. Which this code doesn't do (And can't do (or anything like it) because it would cause spurious remount failures just because someone execs something.). So 0fd338b2d2cd ("exec: move path_noexec() check earlier") introduced WARN_ON_ONCE(). I suspect that it simply wasn't clear that mount properties can change while a is file held open if no write access was claimed. Stuff like noexec, nodev or whatever can change anytime if e.g., just read access was requested. In other words, successful permission checking during path lookup doesn't mean that permission checking wouldn't fail if one redid the checks immediately after. I think it probably never triggered because noexec -> exec remounts are rarely done and the timing would have to be rather precise. I think the immediate solution is to limit the scope of the WARN_ON_ONCE() to the ->i_mode check. diff --git a/fs/exec.c b/fs/exec.c index a126e3d1cacb..12914e14132d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -145,13 +145,14 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) goto out; /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. + * Safety paranoia: Redo the check whether the mount isn't + * noexec so it's as close the the actual open() as possible. + * may_open() has already check this but the mount properties + * may have already changed since then. */ - error = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) + err = -EACCES; + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)) || + path_noexec(&file->f_path)) goto exit; error = -ENOEXEC; @@ -974,13 +975,14 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) goto out; /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. + * Safety paranoia: Redo the check whether the mount isn't + * noexec so it's as close the the actual open() as possible. + * may_open() has already check this but the mount properties + * may have already changed since then. */ err = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)) || + path_noexec(&file->f_path)) goto exit; out: