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 3B6DAC433F5 for ; Thu, 20 Jan 2022 16:37:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B1D2A6B009C; Thu, 20 Jan 2022 11:37:13 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id ACCB96B00A0; Thu, 20 Jan 2022 11:37:13 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 994876B00A1; Thu, 20 Jan 2022 11:37:13 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0233.hostedemail.com [216.40.44.233]) by kanga.kvack.org (Postfix) with ESMTP id 8A3646B009C for ; Thu, 20 Jan 2022 11:37:13 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4A59A8249980 for ; Thu, 20 Jan 2022 16:37:13 +0000 (UTC) X-FDA: 79051220346.25.F481484 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf10.hostedemail.com (Postfix) with ESMTP id 4701EC001D for ; Thu, 20 Jan 2022 16:37:12 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 84CF0ED1; Thu, 20 Jan 2022 08:37:11 -0800 (PST) Received: from [10.57.68.26] (unknown [10.57.68.26]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AC96A3F73D; Thu, 20 Jan 2022 08:37:08 -0800 (PST) Message-ID: Date: Thu, 20 Jan 2022 16:37:01 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] vmap(): don't allow invalid pages Content-Language: en-GB To: "Russell King (Oracle)" Cc: Matthew Wilcox , Yury Norov , Catalin Marinas , Will Deacon , Andrew Morton , Nicholas Piggin , Ding Tianhong , Anshuman Khandual , Alexey Klimov , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20220118235244.540103-1-yury.norov@gmail.com> <319b09bc-56a2-207f-6180-3cc7d8cd43d1@arm.com> From: Robin Murphy In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 4701EC001D X-Stat-Signature: ikghc83qkzzwqcfhp3cgfgssxnhz5jrk Authentication-Results: imf10.hostedemail.com; dkim=none; spf=pass (imf10.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com; dmarc=pass (policy=none) header.from=arm.com X-HE-Tag: 1642696632-532808 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 2022-01-20 13:03, Russell King (Oracle) wrote: > On Thu, Jan 20, 2022 at 12:22:35PM +0000, Robin Murphy wrote: >> On 2022-01-19 19:12, Russell King (Oracle) wrote: >>> On Wed, Jan 19, 2022 at 06:43:10PM +0000, Robin Murphy wrote: >>>> Indeed, my impression is that the only legitimate way to get hold of a page >>>> pointer without assumed provenance is via pfn_to_page(), which is where >>>> pfn_valid() comes in. Thus pfn_valid(page_to_pfn()) really *should* be a >>>> tautology. >>> >>> That can only be true if pfn == page_to_pfn(pfn_to_page(pfn)) for all >>> values of pfn. >>> >>> Given how pfn_to_page() is defined in the sparsemem case: >>> >>> #define __pfn_to_page(pfn) \ >>> ({ unsigned long __pfn = (pfn); \ >>> struct mem_section *__sec = __pfn_to_section(__pfn); \ >>> __section_mem_map_addr(__sec) + __pfn; \ >>> }) >>> #define page_to_pfn __page_to_pfn >>> >>> that isn't the case, especially when looking at page_to_pfn(): >>> >>> #define __page_to_pfn(pg) \ >>> ({ const struct page *__pg = (pg); \ >>> int __sec = page_to_section(__pg); \ >>> (unsigned long)(__pg - __section_mem_map_addr(__nr_to_section(__sec))); \ >>> }) >>> >>> Where: >>> >>> static inline unsigned long page_to_section(const struct page *page) >>> { >>> return (page->flags >> SECTIONS_PGSHIFT) & SECTIONS_MASK; >>> } >>> >>> So if page_to_section() returns something that is, e.g. zero for an >>> invalid page in a non-zero section, you're not going to end up with >>> the right pfn from page_to_pfn(). >> >> Right, I emphasised "should" in an attempt to imply "in the absence of >> serious bugs that have further-reaching consequences anyway". >> >>> As I've said now a couple of times, trying to determine of a struct >>> page pointer is valid is the wrong question to be asking. >> >> And doing so in one single place, on the justification of avoiding an >> incredibly niche symptom, is even more so. Not to mention that an address >> size fault is one of the best possible outcomes anyway, vs. the untold >> damage that may stem from accesses actually going through to random parts of >> the physical memory map. > > I don't see it as a "niche" symptom. The commit message specifically cites a Data Abort "at address translation later". Broadly speaking, a Data Abort due to an address size fault only occurs if you've been lucky enough that the bogus PA which got mapped is so spectacularly wrong that it's beyond the range configured in TCR.IPS. How many other architectures even have a mechanism like that? > If we start off with the struct page being invalid, then the result of > page_to_pfn() can not be relied upon to produce something that is > meaningful - which is exactly why the vmap() issue arises. > > With a pfn_valid() check, we at least know that the PFN points at > memory. No, we know it points to some PA space which has a struct page to represent it. pfn_valid() only says that pfn_to_page() will yield a valid result. That also includes things like reserved pages covering non-RAM areas, where a kernel VA mapping existing at all could potentially be fatal to the system even if it's never explicitly accessed - for all we know it might be a carveout belonging to overly-aggressive Secure software such that even a speculative prefetch might trigger an instant system reset. > However, that memory could be _anything_ in the system - it > could be the kernel image, and it could give userspace access to > change kernel code. > > So, while it is useful to do a pfn_valid() check in vmap(), as I said > to willy, this must _not_ be the primary check. It should IMHO use > WARN_ON() to make it blatently obvious that it should be something we > expect _not_ to trigger under normal circumstances, but is there to > catch programming errors elsewhere. Rather, "to partially catch unrelated programming errors elsewhere, provided the buggy code happens to call vmap() rather than any of the many other functions with a struct page * argument." That's where it stretches my definition of "useful" just a bit too far. It's not about perfect being the enemy of good, it's about why vmap() should be special, and death by a thousand "useful" cuts - if we don't trust the pointer, why not check its alignment for basic plausibility first? If it seems valid, why not check if the page flags look sensible to make sure? How many useful little checks is too many? Every bit of code footprint and execution overhead imposed unconditionally on all end users to theoretically save developers' debugging time still adds up. Although on that note, it looks like arch/arm's pfn_valid() is still a linear scan of the memblock array, so the overhead of adding that for every page in every vmap() might not even be so small... Robin.