From patchwork Mon Aug 5 13:17:21 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mateusz Guzik X-Patchwork-Id: 13753620 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 348BBC3DA4A for ; Mon, 5 Aug 2024 13:17:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A48616B00A2; Mon, 5 Aug 2024 09:17:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9F8F36B00A3; Mon, 5 Aug 2024 09:17:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 898726B00A4; Mon, 5 Aug 2024 09:17:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 6AF936B00A2 for ; Mon, 5 Aug 2024 09:17:37 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id D7DC141D53 for ; Mon, 5 Aug 2024 13:17:36 +0000 (UTC) X-FDA: 82418243712.07.A30D468 Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by imf03.hostedemail.com (Postfix) with ESMTP id 06CAC20012 for ; Mon, 5 Aug 2024 13:17:34 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SNc9kd3R; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.208.41 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722863847; a=rsa-sha256; cv=none; b=SUpvs/HsLH05qBPO8DzMY6GfUu/W6wZDkqMD6zBgoEtX2Mm56qTrqhJXY97Md0IqkB8HW+ knv+DuIvSZ3XyPn5sYoCkXNc73uMH5HiUZn+ssZQamwWAfp/mNLWf+Vn2/OZWfKEh4R2s6 0eqZ0iVVJeVowSbzdpyMV6X56vcDNpI= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SNc9kd3R; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.208.41 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722863847; 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-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=lDNSEMAE3S30qdcaG1s1hlYV7GH7sGqMiEO4UvbDEpA=; b=MWBjTEqofBJnII6xZQS6xpc9wD9c9PMAhNRarQDVK4vDRiyOMqMl1DkbRKvzUwRWcPnztO wG4O0SLzfLLIOxcFKT4npmSa4Snczii5qqdWIlZ8dAPoBJlrCeVw9KljcuCqW9Y42yZjNk DL2OUsMzfR2Ob75gxKuPbwBcSNcbKfo= Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-5b5b67d0024so7812297a12.0 for ; Mon, 05 Aug 2024 06:17:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1722863853; x=1723468653; darn=kvack.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=lDNSEMAE3S30qdcaG1s1hlYV7GH7sGqMiEO4UvbDEpA=; b=SNc9kd3Rcv7Ta+LB1fq6YvnXRnAek2eG81Dz8MH+LszbLgBmUDYWvr/1vyPW2QJoYx 97kjSPll3rPgC4poe+052zYKlBZjQZQKvZ5cdc81N3GwZNq+y/OwDn8hKxtrSNDgaXX/ VjxogblRINoRQcArLN7UD/AP27mAyWsFpMYFHm2QRl/4ZJ86utjlJheqpMsS279V9MRp jpjAtE4vecrqPK+zwxEr1X87ODdvSuVKRHTx5zcqEVv3Ci8Mydgst3K+GX0IZ/Avo9OX WQbEVp8NZ3IhuXMLzfLFZU/D/ItlLX96Il7Y2bhRknMOu9xz7yskkk166IioUIlPsA+o yV5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722863853; x=1723468653; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=lDNSEMAE3S30qdcaG1s1hlYV7GH7sGqMiEO4UvbDEpA=; b=IyHYvEj0q6VFDbma4m8twMkxESLuzmIFJ98MFF0nAWJdeE7hqWmY7xtbI+9qKiaE1a iAvsrmI36fSmIoDhQjJDemwhKNXSj3P9kdYcBiF+FmYBIP9IriEdExWZPrkAOf74/Vq+ k8hKXHpUSmtGlmMfHwkQnt5fbt0U0mWK3iaIjXAgKlIjE4WX4XZGkIOggBeseADJ1l1t +k5ierqv/tgiRYcUpw24WskbhJ40iRuOe1jnlkHFiUoOXeTxvN9Jk6CjwLZgwQCg9frY 29Bhal58I11QOj8MTTVfu48vkuP+ffcRYxWiNE3AYJpkqxLiEGijt2C/wjVjopvxYpg7 VogA== X-Forwarded-Encrypted: i=1; AJvYcCVHacFb79KueMrNZV59Q6cnPZ3NXroE4SWC+nXQuPh3UEQB/RyOwH/hHRWOxNZYklKekSuCZx9RvUimbOGZ5RUSgrg= X-Gm-Message-State: AOJu0YxGqUhs+9RM2rzfERsWrCo9m5u1DrUjxAism5XMlioWceg6xL2L PxqmKQtj0kceJmUvnfg9ey6435OS8qaxbMs7pClj0+sOt1SRcPtZ X-Google-Smtp-Source: AGHT+IGM3UFviiotd+JDfgUlzx9hTo0C30VIkxQjouHOKlUGkBx/6M8kqAKzAQIYwJXdATLI1VVQHA== X-Received: by 2002:a50:e602:0:b0:5a2:763e:b8bf with SMTP id 4fb4d7f45d1cf-5b7f53146d9mr7942354a12.25.1722863853055; Mon, 05 Aug 2024 06:17:33 -0700 (PDT) Received: from f.. (cst-prg-90-207.cust.vodafone.cz. [46.135.90.207]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5b83b82c210sm4893206a12.62.2024.08.05.06.17.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 05 Aug 2024 06:17:32 -0700 (PDT) From: Mateusz Guzik To: brauner@kernel.org Cc: viro@zeniv.linux.org.uk, jack@suse.cz, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, josef@toxicpanda.com, wojciech.gladysz@infogain.com, ebiederm@xmission.com, kees@kernel.org, linux-mm@kvack.org, Mateusz Guzik Subject: [PATCH] exec: drop a racy path_noexec check Date: Mon, 5 Aug 2024 15:17:21 +0200 Message-ID: <20240805131721.765484-1-mjguzik@gmail.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240805-fehlbesetzung-nilpferd-1ed58783ad4d@brauner> References: <20240805-fehlbesetzung-nilpferd-1ed58783ad4d@brauner> MIME-Version: 1.0 X-Rspam-User: X-Rspamd-Queue-Id: 06CAC20012 X-Rspamd-Server: rspam01 X-Stat-Signature: kog1aiujk7uosnmbu59icqba53i453wm X-HE-Tag: 1722863854-801730 X-HE-Meta: U2FsdGVkX1/So9r41igYuVCwlStIHWpYjmT71DT+9lyRk86/nQa8NhMZoHVB/FetJxwYVCieHruMtPNb5gr9Vzvnu+LJSs1BjHHwhYWi3Lj4A1wqnh0lMel8H1XkoMXljAYfbvq4kJIiCjPXIA7eprrnpY/EvH6DT8uCrA88U6Dr+UjZGICxnpMYyPp3BKggy7JEn/1wH5+lpsH+RQg1D26ea1SIHBzYIhMt0dd6Bg1rItHKCGKjlNpwx7LPkr7fCtPDUr0duZ14vx/LxSsl1l1NAQ5QxAZSfzB7fcCTEeVZGp0OpeaMVe2+cIT6QKyIqxuSyH1mR3VsT4PqXHi49Ks2UD6xlNKVQ0mXAwG0MBZFPscRmMoSMwBHBtL/HdxLLnfJhmU+G55jaeL4RkOCQ9SnfNsYrfhvU0zjM2Yp336dT5mBORHuNE8TIw8EY2MX5gT3s96CvV41d7rxkgXSKK3XgkkwZgo8wnMGLcHBqB0vCwCizP1FHLWU1pbotJ2+NeD70dAQ8BPXlz3i1p5jOBsVe1Vrtwps/mNXFouUphWlF2B4gn7vBDTgfLIkSAC5kzrlYmyY8u4+GiA2lhNGwQ4I//iJeQC1oJ+fB7/s6DcF93/wUg5QQuNRVM+egZoegTWgaeQEWbftK/7oEsuQZiAoEdCKb/UkNHjc/plcCXSxMHo/LzQDlCF/ZJ3deSrDYYrUiOAe9sdZS7WwzerDvWUR+iyotCXSt80+13VXGy1JNAVvFljJSqu5T4qwTLPNbhTZPdo9TBO+JcLz92KuJWwYEWf7eb4blTZPWYWiRe5Pq+rg33QIYkAgyo8T2VRa68BHIlaAFPrpvGWk3bWgdhcK+0M80aSGV+BsTXdKPV6JbEEPxECvkz7KishU9utxBsEeD0Px5OQmvjuywQngZfrdb8d4ZU8S0xLzvIfhE0v2GM2LHb7FN8Si4s25gn3KL81ZusRd2cZpKOqcT3J FFqrbp31 gKtIvsnYXANGS6DLH0oks6kmDuY+6OyNDdBQ/CLHXlq8uFi+0RbVNBLRrT16br+823T7n8D1GlKtiny2Od2LJVbhFdlhIl1Id+/KSWWEZRk8YuIzShr1bC2T87O8aO9qgg6+S5fzdE/53Ym+QG+dKwZ/et1B5u8Nf7U2L3JV/rZIluis1Fz7fJyb1oHpfDQk7EZGBp1IsA7C2anUgWNZYarGUaPVnrty8vohFvA5KuNX7UhVw0Y6cevqjm5yM4aYXIeTYeetWu4WsItC0LQDgJ4Zqw1nKWLKkHGPKnx895FzHVhpoLRVwgBKjm4mupDSZDJLyvQHS2+ypYS2J8w90G0yJuOxs7TpC7OGAzZmtAnrCKCDrlVc+DKS7fknuzk2Elh/bpN7UhpMX8+hAzftXPj3S0w== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000087, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Both i_mode and noexec checks wrapped in WARN_ON stem from an artifact of the previous implementation. They used to legitimately check for the condition, but that got moved up in two commits: 633fb6ac3980 ("exec: move S_ISREG() check earlier") 0fd338b2d2cd ("exec: move path_noexec() check earlier") Instead of being removed said checks are WARN_ON'ed instead, which has some debug value However, the spurious path_noexec check is racy, resulting in unwarranted warnings should someone race with setting the noexec flag. One can note there is more to perm-checking whether execve is allowed and none of the conditions are guaranteed to still hold after they were tested for. Additionally this does not validate whether the code path did any perm checking to begin with -- it will pass if the inode happens to be regular. As such remove the racy check. The S_ISREG thing is kept for the time being since it does not hurt. Reword the commentary and do small tidy ups while here. Signed-off-by: Mateusz Guzik --- On Mon, Aug 05, 2024 at 11:26:19AM +0200, Christian Brauner wrote: > I think the immediate solution is to limit the scope of the > WARN_ON_ONCE() to the ->i_mode check. > To my reading that path_noexec is still there only for debug, not because of any security need. To that end just I propose just whacking it. fs/exec.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index a126e3d1cacb..2938cbe38343 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -145,13 +145,10 @@ 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. + * Check do_open_execat() for an explanation. */ error = -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))) goto exit; error = -ENOEXEC; @@ -954,7 +951,6 @@ EXPORT_SYMBOL(transfer_args_to_stack); static struct file *do_open_execat(int fd, struct filename *name, int flags) { struct file *file; - int err; struct open_flags open_exec_flags = { .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, .acc_mode = MAY_EXEC, @@ -971,24 +967,21 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) file = do_filp_open(fd, name, &open_exec_flags); if (IS_ERR(file)) - goto out; + return file; /* - * 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. + * Validate the type. + * + * In the past the regular type check was here. It moved to may_open() in + * 633fb6ac3980 ("exec: move S_ISREG() check earlier"). Since then it is + * an invariant that all non-regular files error out before we get here. */ - err = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) - goto exit; + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode))) { + fput(file); + return ERR_PTR(-EACCES); + } -out: return file; - -exit: - fput(file); - return ERR_PTR(err); } /**