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 X-Spam-Level: X-Spam-Status: No, score=-5.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4501CC432BE for ; Thu, 12 Aug 2021 19:38:36 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CEF3F6103A for ; Thu, 12 Aug 2021 19:38:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org CEF3F6103A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 1C31D8D0002; Thu, 12 Aug 2021 15:38:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 173228D0001; Thu, 12 Aug 2021 15:38:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 014498D0002; Thu, 12 Aug 2021 15:38:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0094.hostedemail.com [216.40.44.94]) by kanga.kvack.org (Postfix) with ESMTP id DBF3A8D0001 for ; Thu, 12 Aug 2021 15:38:34 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 7FFA6180364D5 for ; Thu, 12 Aug 2021 19:38:34 +0000 (UTC) X-FDA: 78467440548.16.E207C74 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf20.hostedemail.com (Postfix) with ESMTP id 294D6D008C7B for ; Thu, 12 Aug 2021 19:38:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1628797113; h=from:from: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; bh=tGJibS4yiaglb2J5rU+JWBqNKg1kdcrr0OdS+mMkHrI=; b=YsZPejHEDNuP59u5DgBvk4kho1KqLMikTMva8fQFqGcUB5Zv/X5np47jKoEAhQC2o1/3fw XN1GGdkS/lNaYiTeY0hEC4I1CJpgS5MlUQCo/bXS7Tot3QgIFzp+/Wbu7B36XlIjkIkYI3 7taUgRmjsZlLW+RzVkCA0VgaR/0r4K4= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-190-GNTrkJG7MkuC5wLaCStYTw-1; Thu, 12 Aug 2021 15:38:32 -0400 X-MC-Unique: GNTrkJG7MkuC5wLaCStYTw-1 Received: by mail-wr1-f72.google.com with SMTP id t15-20020a5d42cf000000b001565f9c9ee8so821wrr.2 for ; Thu, 12 Aug 2021 12:38:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:organization:subject :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=tGJibS4yiaglb2J5rU+JWBqNKg1kdcrr0OdS+mMkHrI=; b=i4+KhYlp48J7tADwMFCElUKHc6QVyMrZpJsuY3yF2N4fYc3qXT0kseI+eCwpjKacXX 77oefDAiXfcHAh9d+RDl4xJArkaoMBpzA3/P58XxoozqkSOsVdvk3M5PLzJVMvr4Hx+D usJ4WVrgie6Jlwg+41rO/aHTERucpKgaTXn4g/NGuVRpVSyGRwNLA7aJGIk2bUW/vCcT Nc8e23o3uIh6bY1JLcARiO8NjslOFvCrHbhDHnnIRdDgylAzWuYlPe13c7A9q1c1NQl3 r5A4BnrwWz1KgUdde2+HlESBtL3alEyNLeU9ltMZXZa8KrJzPUt0Sc1Zs6R5WCBdV2Tg cSdA== X-Gm-Message-State: AOAM5326lyxob/0jc0Z6ZWogSXFE56KJXo2DKVKp9qaKavdZYMaO3f0D O6MlGbU2nm1b97a82RMLmiRfleQFKwInDi9EZ63QOJHdq23dLQo4zw3Hlxp00JGMSj1GJOZTI5p UFo67QCgmXUtqpMSxzIlHfD2RqjFWdKHAwHEUkktSBPnYDtXzJyorLhClqxc= X-Received: by 2002:a5d:45c2:: with SMTP id b2mr5697510wrs.188.1628797111290; Thu, 12 Aug 2021 12:38:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw2yA9S5GLR5aZS72Vs7D04/GT/ipvgAFBAJrXhJd9/Q2ngghrrOQ4av0TFSdLK0A6HDNvrvg== X-Received: by 2002:a5d:45c2:: with SMTP id b2mr5697441wrs.188.1628797110959; Thu, 12 Aug 2021 12:38:30 -0700 (PDT) Received: from [192.168.3.132] (p4ff23d8b.dip0.t-ipconnect.de. [79.242.61.139]) by smtp.gmail.com with ESMTPSA id i9sm4899610wre.36.2021.08.12.12.38.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 12 Aug 2021 12:38:29 -0700 (PDT) To: Linus Torvalds Cc: Linux Kernel Mailing List , Andrew Morton , Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , Alexander Viro , Alexey Dobriyan , Steven Rostedt , Peter Zijlstra , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Petr Mladek , Sergey Senozhatsky , Andy Shevchenko , Rasmus Villemoes , Kees Cook , "Eric W. Biederman" , Greg Ungerer , Geert Uytterhoeven , Mike Rapoport , Vlastimil Babka , Vincenzo Frascino , Chinwen Chang , Michel Lespinasse , Catalin Marinas , "Matthew Wilcox (Oracle)" , Huang Ying , Jann Horn , Feng Tang , Kevin Brodsky , Michael Ellerman , Shawn Anastasio , Steven Price , Nicholas Piggin , Christian Brauner , Jens Axboe , Gabriel Krisman Bertazi , Peter Xu , Suren Baghdasaryan , Shakeel Butt , Marco Elver , Daniel Jordan , Nicolas Viennot , Thomas Cedeno , Collin Fijalkovich , Michal Hocko , Miklos Szeredi , Chengguang Xu , =?UTF-8?Q?Christian_K=c3=b6nig?= , linux-unionfs@vger.kernel.org, Linux API , the arch/x86 maintainers , linux-fsdevel , Linux-MM References: <20210812084348.6521-1-david@redhat.com> <20210812084348.6521-4-david@redhat.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v1 3/7] kernel/fork: always deny write access to current MM exe_file Message-ID: <15628c8a-9c71-5611-2edf-07087ad662b7@redhat.com> Date: Thu, 12 Aug 2021 21:38:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=YsZPejHE; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf20.hostedemail.com: domain of david@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=david@redhat.com X-Stat-Signature: ign4w9tih1jnkgyrkiusbgpffshuaqqh X-Rspamd-Queue-Id: 294D6D008C7B X-Rspamd-Server: rspam05 X-HE-Tag: 1628797114-988649 Content-Transfer-Encoding: quoted-printable 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 12.08.21 18:51, Linus Torvalds wrote: > On Wed, Aug 11, 2021 at 10:45 PM David Hildenbrand w= rote: >> >> /* No ordering required: file already has been exposed. */ >> - RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm)); >> + exe_file =3D get_mm_exe_file(oldmm); >> + RCU_INIT_POINTER(mm->exe_file, exe_file); >> + if (exe_file) >> + deny_write_access(exe_file); >=20 > Can we make a helper function for this, since it's done in two differen= t places? Sure, no compelling reason not to (except finding a suitable name, but=20 I'll think about that tomorrow). >=20 >> - if (new_exe_file) >> + if (new_exe_file) { >> get_file(new_exe_file); >> + /* >> + * exec code is required to deny_write_access() succes= sfully, >> + * so this cannot fail >> + */ >> + deny_write_access(new_exe_file); >> + } >> rcu_assign_pointer(mm->exe_file, new_exe_file); >=20 > And the above looks positively wrong. The comment is also nonsensical, > in that it basically says "we thought this cannot fail, so we'll just > rely on it". Well, it documents the expectation towards the caller, but in a=20 suboptimal way, I agree. >=20 > If it truly cannot fail, then the comment should give the reason, not > the "we depend on this not failing". Right, "We depend on the caller already have done a deny_write_access()=20 successfully first such that this call cannot fail." combined with if (deny_write_access(new_exe_file)) pr_warn("Unexpected failure of deny_write_access() in %s", __func__); suggestions welcome. >=20 > And honestly, I don't see why it couldn't fail. And if it *does* fail, > we cannot then RCU-assign the exe_file pointer with this, because > you'll get a counter imbalance when you do the allow_write_access() > later. Anyone calling set_mm_exe_file() (-> begin_new_exec()) is expected to=20 successfully triggered a deny_write_access() upfront such that we won't=20 fail at that point. Further, on the dup_mmap() path we are sure the previous oldmm exe_file=20 properly saw a successful deny_write_access() already, because that's=20 now guaranteed for any exe_file. >=20 > Anyway, do_open_execat() does do deny_write_access() with proper error > checking. I think that is the existing reference that you depend on - > so that it doesn't fail. So the comment could possibly say that the > only caller has done this, but can we not just use the reference > deny_write_access() directly, and not do a new one here? I think that might over-complicate the exec code where we would see a=20 allow_write_access() on error paths, but not on success paths. This here=20 looks cleaner to me, agreeing that the comment and the error check has=20 to be improved. We handle all allow_write_access()/deny_write_access() regarding=20 exe_file completely in kernel/fork.c, which is IMHO quite nice. >=20 > IOW, maybe there's an extraneous 'allow_write_access()' somewhere that > should be dropped when we do the whole binprm dance in execve()? fs/exec.c: free_bprm() and exec_binprm() to be precise. Thanks! --=20 Thanks, David / dhildenb