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 C3E44C19F2D for ; Tue, 9 Aug 2022 20:00:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 19E2F8E0002; Tue, 9 Aug 2022 16:00:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 14E828E0001; Tue, 9 Aug 2022 16:00:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 016588E0002; Tue, 9 Aug 2022 16:00:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E778C8E0001 for ; Tue, 9 Aug 2022 16:00:54 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id B9F1AA1219 for ; Tue, 9 Aug 2022 20:00:54 +0000 (UTC) X-FDA: 79781122428.21.3569CAE Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) by imf30.hostedemail.com (Postfix) with ESMTP id 313A88017D for ; Tue, 9 Aug 2022 20:00:54 +0000 (UTC) Received: by mail-ej1-f53.google.com with SMTP id tl27so24124015ejc.1 for ; Tue, 09 Aug 2022 13:00:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=3Nw+UCMkb8eM5P4o4fg5DPpCP6PyLs67xhLCM5SqQGM=; b=VRBM9D6pcPFAJOfjA+37UektR0jrpOFKofLKXQ8dh9+GHqA9kghDd4C5F1PQSSakLC uiJ/X8JuI7hj3D7sznroAaF0LHsLxleBjlrNKIUQ1Fvc76loYqdCHQw/er29PAcKm6dz RWHrVlurVBlAOBYpfxEdiWKtz+KG/FFvLYv3A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=3Nw+UCMkb8eM5P4o4fg5DPpCP6PyLs67xhLCM5SqQGM=; b=vO+pXyE0MSSxEIjvJPUC2voE2FgoZQCLd3T0IngN7KT2dhEO64jAWnK+BzUIrG+7Gr t5FGR7wE0LBJaGDo0806QW3RjcVTfdkmqQ/1GqJ8Fh9z3Vb6WXE+4c8CgQ52IZbpKbaQ QheCrNmbYaEi0tPi8WjpSIBP3nmsgB4YdJpLc3gwDa4wMs1Rp4J6gK2hupFRSF0HkWPq OpxUv5qOm1w1DPCZGv5OCism+dHoVcSZo9grrTlhNGU+PIIDGd9+Xx2UpV/09t5e1QdI Dj1JfpTboe9XFTZxUqAP9IadsFvov2aZW8un4uAUWggSomufUjROzz/d7vZ+5CDxphsc nHLQ== X-Gm-Message-State: ACgBeo0OIINWzagUDbPxv0V5QtU+t/xT8Q+C8BtNWzW1mS/9d8npb8F3 LsiMFcDXmXiWt1OOEZO5GbPRsMntHp7qT2cCp54= X-Google-Smtp-Source: AA6agR7jecnuDCJrY5FrCO49L8oqa6dao599K2lp9GQ3VpePQJfzF5gNX7/jnWf1iz6HGhLvjEHLnw== X-Received: by 2002:a17:906:98c7:b0:730:e4be:7497 with SMTP id zd7-20020a17090698c700b00730e4be7497mr17091996ejb.347.1660075252256; Tue, 09 Aug 2022 13:00:52 -0700 (PDT) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com. [209.85.221.44]) by smtp.gmail.com with ESMTPSA id g18-20020a17090604d200b0072b3406e9c2sm1464433eja.95.2022.08.09.13.00.51 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 09 Aug 2022 13:00:51 -0700 (PDT) Received: by mail-wr1-f44.google.com with SMTP id h13so15479155wrf.6 for ; Tue, 09 Aug 2022 13:00:51 -0700 (PDT) X-Received: by 2002:a5d:56cf:0:b0:21e:ce64:afe7 with SMTP id m15-20020a5d56cf000000b0021ece64afe7mr14978811wrw.281.1660075250807; Tue, 09 Aug 2022 13:00:50 -0700 (PDT) MIME-Version: 1.0 References: <20220808073232.8808-1-david@redhat.com> In-Reply-To: <20220808073232.8808-1-david@redhat.com> From: Linus Torvalds Date: Tue, 9 Aug 2022 13:00:34 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v1] mm/gup: fix FOLL_FORCE COW security issue and remove FOLL_COW To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, stable@vger.kernel.org, Andrew Morton , Greg Kroah-Hartman , Axel Rasmussen , Peter Xu , Hugh Dickins , Andrea Arcangeli , Matthew Wilcox , Vlastimil Babka , John Hubbard , Jason Gunthorpe Content-Type: text/plain; charset="UTF-8" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660075254; 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=3Nw+UCMkb8eM5P4o4fg5DPpCP6PyLs67xhLCM5SqQGM=; b=8hT8yeppXkUgAiZwTprBXLGkGckbK2BxBDg8utn6hLC99WLv6nQ6qkpljvLiKlIsFMx6z9 KsFaXKXeZffoI0agBC3RRbBxtCqKzrD9IaB6i63zO1668yxoTDI1qEAtBZd7CzC2cnjcMX Sve7VyyHhamn2SGtttYNz/cBnahwAjM= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=VRBM9D6p; dmarc=none; spf=pass (imf30.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.218.53 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660075254; a=rsa-sha256; cv=none; b=cmDWHs2DyGSGqV9z/E3alxR32t8TX2AmwcJ5KFL5+WUpQ+z5Oj1ULv87WE9wVWWy+XDV73 EMvIE8rLQAkynXpfEHwjfEWyw5I7KNcVGn9DwGwZC/0BdqILxxluhNmwsFxFnMed7qqT+i 9KH+sRQ1EKVp6g0yii+2k4/KzAZfF/c= X-Rspamd-Queue-Id: 313A88017D Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=VRBM9D6p; dmarc=none; spf=pass (imf30.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.218.53 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: yzyozjjqkbwbaergbxqtxtsz75nmac61 X-HE-Tag: 1660075254-849425 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 Mon, Aug 8, 2022 at 12:32 AM David Hildenbrand wrote: > So I've read through the patch several times, and it seems fine, but this function (and the pmd version of it) just read oddly to me. > +static inline bool can_follow_write_pte(pte_t pte, struct page *page, > + struct vm_area_struct *vma, > + unsigned int flags) > +{ > + if (pte_write(pte)) > + return true; > + if (!(flags & FOLL_FORCE)) > + return false; > + > + /* > + * See check_vma_flags(): only COW mappings need that special > + * "force" handling when they lack VM_WRITE. > + */ > + if (vma->vm_flags & VM_WRITE) > + return false; > + VM_BUG_ON(!is_cow_mapping(vma->vm_flags)); So apart from the VM_BUG_ON(), this code just looks really strange - even despite the comment. Just conceptually, the whole "if it's writable, return that you cannot follow it for a write" just looks so very very strange. That doesn't make the code _wrong_, but considering how many times this has had subtle bugs, let's not write code that looks strange. So I would suggest that to protect against future bugs, we try to make it be fairly clear and straightforward, and maybe even a bit overly protective. For example, let's kill the "shared mapping that you don't have write permissions to" very explicitly and without any subtle code at all. The vm_flags tests are cheap and easy, and we could very easily just add some core ones to make any mistakes much less critical. Now, making that 'is_cow_mapping()' check explicit at the very top of this would already go a long way: /* FOLL_FORCE for writability only affects COW mappings */ if (!is_cow_mapping(vma->vm_flags)) return false; but I'd actually go even further: in this case that "is_cow_mapping()" helper to some degree actually hides what is going on. So I'd actually prefer for that function to be written something like /* If the pte is writable, we can write to the page */ if (pte_write(pte)) return true; /* Maybe FOLL_FORCE is set to override it? */ if (flags & FOLL_FORCE) return false; /* But FOLL_FORCE has no effect on shared mappings */ if (vma->vm_flags & MAP_SHARED) return false; /* .. or read-only private ones */ if (!(vma->vm_flags & MAP_MAYWRITE)) return false; /* .. or already writable ones that just need to take a write fault */ if (vma->vm_flags & MAP_WRITE) return false; and the two first vm_flags tests above are basically doing tat "is_cow_mapping()", and maybe we could even have a comment to that effect, but wouldn't it be nice to just write it out that way? And after you've written it out like the above, now that if (!page || !PageAnon(page) || !PageAnonExclusive(page)) return false; makes you pretty safe from a data sharing perspective: it's most definitely not a shared page at that point. So if you write it that way, the only remaining issues are the magic special soft-dirty and uffd ones, but at that point it's purely about the semantics of those features, no longer about any possible "oh, we fooled some shared page to be writable". And I think the above is fairly legible without any subtle cases, and the one-liner comments make it all fairly clear that it's testing. Is any of this in any _technical_ way different from what your patch did? No. It's literally just rewriting it to be a bit more explicit in what it is doing, I think, and it makes that odd "it's not writable if VM_WRITE is set" case a bit more explicit. Hmm? Linus