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 B900FC433EF for ; Wed, 19 Jan 2022 17:00:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 344796B0072; Wed, 19 Jan 2022 12:00:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F4016B0073; Wed, 19 Jan 2022 12:00:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1BB6C6B0074; Wed, 19 Jan 2022 12:00:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0096.hostedemail.com [216.40.44.96]) by kanga.kvack.org (Postfix) with ESMTP id 0C33A6B0072 for ; Wed, 19 Jan 2022 12:00:40 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C234081602AF for ; Wed, 19 Jan 2022 17:00:39 +0000 (UTC) X-FDA: 79047650598.15.2167A5D Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by imf31.hostedemail.com (Postfix) with ESMTP id 5463620011 for ; Wed, 19 Jan 2022 17:00:39 +0000 (UTC) Received: by mail-pj1-f41.google.com with SMTP id d12-20020a17090a628c00b001b4f47e2f51so3957953pjj.3 for ; Wed, 19 Jan 2022 09:00:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+miCtcK/AqsybC9kGvXCIMuIzeqRj4OXN3BobeqDEUo=; b=K/2ZjAm2fs8HahIuAat0NhAX7Ei2c00JN0IdOuUX6t/dhtEaL0qFZGuJNqVgz3Wbq1 DqNKYBCTQb/g1sx8TKs0O2wLWqp+uxKxq/LY8gJhZRW2vZOIh23E8TpTLtAYWgCE81OO 6w1Q9ANCTvlLhhwW4UB7d8ewEeRBtPI6uFwqmeguWR/38nm4Swb+wjdjV3rPE7nVZve7 ltctmU0Fgzo8VB8bVaFPhvgP/gDGpnN7NCJNXIROghFiQhG6LAhbE9fwanNHPgApnvBw 00l8vG6tZoyKG3hqgsaP7YL2l+F01JoPtmNAICd9dlzogLjq9KlYSSC56iS0F7Qhzl6O WZSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+miCtcK/AqsybC9kGvXCIMuIzeqRj4OXN3BobeqDEUo=; b=xGAIFaeeMX0KrTGDxGJMDM1dOW+iNGWzlFMwDYVBbpFTjTxAhUkojzVwOEVU+JcklL un3zmctnBqcGaKK4LAeWHA4vREGv8GeTjb3idkWl/zi7sje7T7qnxydOIHtrQpfN1lnv aLU4crC33Elk0eMXWbXmx9OI/enrX1w2cc5aSZ9WpVYbEG9+ZvIfJ/noKiXDGy77mR+y QMhPBoG6f9WC3BvwGBIQZUU+tTzoofL142w9mW4zdJiaV6R1O0EMEpUg9waTUdjdKcQ3 fk2GuYslLaEHNOEQ4COKkyewDphI9kvUaWFbSE84yo3GaL8d2SnYbtBOeW+AMmbqpjpE mJTA== X-Gm-Message-State: AOAM530MkEW5hkffhkiLyojtNRICs3RvmOEFYxMOjQpbgqzH/5q0eDiR 4ZOv50OtboyyiIZcAapE4+Re6tWHD+1QGvebIqc= X-Google-Smtp-Source: ABdhPJzsPKAdJawi2o/caxCQqkDSAvrXLEzsPlx4tZTMVShOZR6MIF3N0SfPKEZEgr9U4XvOFhfTfZw7NnvadJkckF0= X-Received: by 2002:a17:902:a5c6:b0:149:c926:7c31 with SMTP id t6-20020a170902a5c600b00149c9267c31mr34261842plq.141.1642611638142; Wed, 19 Jan 2022 09:00:38 -0800 (PST) MIME-Version: 1.0 References: <20220118235244.540103-1-yury.norov@gmail.com> In-Reply-To: From: Yury Norov Date: Wed, 19 Jan 2022 09:00:23 -0800 Message-ID: Subject: Re: [PATCH] vmap(): don't allow invalid pages To: Mark Rutland Cc: Catalin Marinas , Will Deacon , Andrew Morton , Nicholas Piggin , Ding Tianhong , Anshuman Khandual , Matthew Wilcox , Alexey Klimov , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 5463620011 X-Stat-Signature: 5xixtfj6g9c8nbz5cp8dwwxhcmwosgqw Authentication-Results: imf31.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="K/2ZjAm2"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf31.hostedemail.com: domain of yury.norov@gmail.com designates 209.85.216.41 as permitted sender) smtp.mailfrom=yury.norov@gmail.com X-Rspamd-Server: rspam03 X-HE-Tag: 1642611639-662440 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 Wed, Jan 19, 2022 at 3:17 AM Mark Rutland wrote: > > Hi, > > I replied ot the original RFC before spotting this; duplicating those comments > here because I think they apply regardless of the mechanism used to work around > this. > > On Tue, Jan 18, 2022 at 03:52:44PM -0800, Yury Norov wrote: > > vmap() takes struct page *pages as one of arguments, and user may provide > > an invalid pointer which would lead to DABT at address translation later. > > > > Currently, kernel checks the pages against NULL. In my case, however, the > > address was not NULL, and was big enough so that the hardware generated > > Address Size Abort on arm64. > > Can you give an example of when this might happen? It sounds like you're > actually hitting this, so a backtrace would be nice. > > I'm a bit confused as to when why we'd try to vmap() pages that we > didn't have a legitimate struct page for -- where did these addresses > come from? > > It sounds like this is going wrong at a higher level, and we're passing > entirely bogus struct page pointers around. This seems like the sort of > thing DEBUG_VIRTUAL or similar should check when we initially generate > the struct page pointer. Hi Mark, This is an out-of-tree code that does: vaddr1 = dma_alloc_coherent() page = virt_to_page() // Wrong here ... vaddr2 = vmap(page) memset(vaddr2) // Fault here virt_to_page() returns a wrong pointer if vaddr1 is not a linear kernel address. The problem is that vmap() populates pte with bad pfn successfully, and it's much harder to debug at memory access time. > > Interestingly, this abort happens even if copy_from_kernel_nofault() is > > used, which is quite inconvenient for debugging purposes. > > I can go take a look at this, but TBH we never expect to take an address size > fault to begin with, so this is arguably correct -- it's an internal > consistency problem. > > > This patch adds a pfn_valid() check into vmap() path, so that invalid > > mapping will not be created. > > > > RFC: https://lkml.org/lkml/2022/1/18/815 > > v1: use pfn_valid() instead of adding an arch-specific > > arch_vmap_page_valid(). Thanks to Matthew Wilcox for the hint. > > > > Signed-off-by: Yury Norov > > --- > > mm/vmalloc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index d2a00ad4e1dd..a4134ee56b10 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -477,6 +477,8 @@ static int vmap_pages_pte_range(pmd_t *pmd, unsigned long addr, > > return -EBUSY; > > if (WARN_ON(!page)) > > return -ENOMEM; > > + if (WARN_ON(!pfn_valid(page_to_pfn(page)))) > > + return -EINVAL; > > My fear here is that for this to fire, we've already passed a bogus struct page > pointer around the intermediate infrastructure, and any of that might try to > use it in unsafe ways (in future even if we don't use it today). > > I think the fundamental issue here is that we generate a bogus struct page > pointer at all, and knowing where that came from would help to fix that. You're right. That's why WARN_ON() is used for the page == null in the code above, I believe, - to let client code know that something goes wrong, and it's not a regular ENOMEM situation. Thanks, Yury > Thanks, > Mark. > > > set_pte_at(&init_mm, addr, pte, mk_pte(page, prot)); > > (*nr)++; > > } while (pte++, addr += PAGE_SIZE, addr != end); > > -- > > 2.30.2 > >