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 43D60C433F5 for ; Thu, 6 Oct 2022 09:05:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A12716B0071; Thu, 6 Oct 2022 05:05:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 999006B0073; Thu, 6 Oct 2022 05:05:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 812A16B0074; Thu, 6 Oct 2022 05:05:20 -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 6B5A86B0071 for ; Thu, 6 Oct 2022 05:05:20 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 35F6A80AB3 for ; Thu, 6 Oct 2022 09:05:20 +0000 (UTC) X-FDA: 79989940800.28.C790796 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf04.hostedemail.com (Postfix) with ESMTP id BA8D04001E for ; Thu, 6 Oct 2022 09:05:18 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C0830B81EA0; Thu, 6 Oct 2022 09:05:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CB455C433D6; Thu, 6 Oct 2022 09:05:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1665047115; bh=3jGXsNPSS4O1TEpS68FiH8Y48vDWXXQybQJHFsdIyaY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CHAgPTyna9bAa5+1gY1y6nPN3rE3uxDZIDS9lvu9KCJX+0UHxssIa95r+hCe0rgsb uCg+oilhWGlEg0HRKfKcRYYzLuUnuRoklCSpB8zkg9W/7UlpdKLCkpo9dB5iNQSAHt q4zZUXAk4CMUMsIn5JKS8i1RMQw7xnCcemwF24WdQNAybg2yzc/SrmqjhygWlrdeA0 gEPZqRhnne1FkqYY5PcU/5ah2xFo86ptFevX/EHKDgV8bAHgI3nJpZJW5DsbTo8fuy 8EPEbJam5U0csRzGMkDqlDA/DadY0A1WEqA16RoGFhSfaMcdf/hMuC3asH4eoy7tTs 5b0y4Sw0n/IGA== Date: Thu, 6 Oct 2022 11:05:06 +0200 From: Christian Brauner To: Kees Cook Cc: Eric Biederman , Jorge Merlino , Alexander Viro , Thomas Gleixner , Andy Lutomirski , Sebastian Andrzej Siewior , Andrew Morton , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, John Johansen , Paul Moore , James Morris , "Serge E. Hallyn" , Stephen Smalley , Eric Paris , Richard Haines , Casey Schaufler , Xin Long , "David S. Miller" , Todd Kjos , Ondrej Mosnacek , Prashanth Prahlad , Micah Morton , Fenghua Yu , Andrei Vagin , linux-kernel@vger.kernel.org, apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec Message-ID: <20221006090506.paqjf537cox7lqrq@wittgenstein> References: <20221006082735.1321612-1-keescook@chromium.org> <20221006082735.1321612-2-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221006082735.1321612-2-keescook@chromium.org> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1665047118; 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=iLRnEPDU3D8syAiwZeLxiQJZtO+fhpjYRgwySUv/G2k=; b=lpPYlAWmQzm+n4JR4hJsnOqFG3pfCCbR7wEO8LpqwtvJ5GS0z1ehQYFF954X89RDB+FiLX NnzN9CTt+Kf/j8rox5tDzAc/pNfFXxI4GRGVTDv9dCEuW+bXdkBzrdsBx8OkAUarunrklG Idzua24uMUR1dmSWIqbuV/dwg886Jeg= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=CHAgPTyn; spf=pass (imf04.hostedemail.com: domain of brauner@kernel.org designates 145.40.68.75 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=1665047118; a=rsa-sha256; cv=none; b=iTeCo8yse/yCP5wUE5jtJ4jH3OSf1xwWzyj3iHVQxLfsSAxIgEBinweFk+Hr5//1boSOSe rmp8vDk3OinI2caSV2hwAgra2rAUNtfx2HljUtecuqn7GubLcL42RWHC5recQVclaFRMFP Wft3V/rPvAidqf/QJPo0HFW2C0fMeHk= X-Rspamd-Queue-Id: BA8D04001E Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=CHAgPTyn; spf=pass (imf04.hostedemail.com: domain of brauner@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=brauner@kernel.org; dmarc=pass (policy=none) header.from=kernel.org X-Rspamd-Server: rspam06 X-Rspam-User: X-Stat-Signature: ciqc8sn5xqp8qwmhiqxsf98aibdkgnsc X-HE-Tag: 1665047118-241170 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: On Thu, Oct 06, 2022 at 01:27:34AM -0700, Kees Cook wrote: > The check_unsafe_exec() counting of n_fs would not add up under a heavily > threaded process trying to perform a suid exec, causing the suid portion > to fail. This counting error appears to be unneeded, but to catch any > possible conditions, explicitly unshare fs_struct on exec, if it ends up Isn't this a potential uapi break? Afaict, before this change a call to clone{3}(CLONE_FS) followed by an exec in the child would have the parent and child share fs information. So if the child e.g., changes the working directory post exec it would also affect the parent. But after this change here this would no longer be true. So a child changing a workding directoro would not affect the parent anymore. IOW, an exec is accompanied by an unshare(CLONE_FS). Might still be worth trying ofc but it seems like a non-trivial uapi change but there might be few users that do clone{3}(CLONE_FS) followed by an exec. > +/* > + * Unshare the filesystem structure if it is being shared > + */ > +int unshare_fs(void) > +{ > + struct fs_struct *new_fs = NULL; > + int error; > + > + error = unshare_fs_alloc(CLONE_FS, &new_fs); > + if (error || !new_fs) > + return error; You should just check for error here, not new_fs.