Received: by 10.192.165.148 with SMTP id m20csp1337678imm; Fri, 27 Apr 2018 18:00:12 -0700 (PDT) X-Google-Smtp-Source: AB8JxZqX6eyCWVKNGFoqGJ2iRauDsSYNDQTe4GWdakYCapLCIfyJDbUNscfIHTUArc+Tkzvl1KOp X-Received: by 2002:a63:ab45:: with SMTP id k5-v6mr1521607pgp.199.1524877212506; Fri, 27 Apr 2018 18:00:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524877212; cv=none; d=google.com; s=arc-20160816; b=jBpjkgiomvsK1uWQMffvtpoYS3yvq1nOr4+f0USkrJy4ErZLr7L2Z3F2ckn5jLvi58 T4JAmnNij7bf6TbqK4QwcfB76D/uGK6eUkmTmQ1VNIHNhamPm7zxnVxA/bMZHgVRVAhV Jr40IkTN9cVN+oFowYWPp26zqSHpgf/myKK8BcxAm6IhiKXNLMrh5UJgAY36TSi5v3N/ IAUCf9qYXgUXhMTC9NF/yns/JD5tP75Sqd21wS282FJoupekXZYQRMcpU50Vj/1y+kQf mUP5GMGAKQaEeUR7dbyeDBdmz4VfBXcSjmXGi7vp2pf9FmMl8FqxDLU7SCzyqBe0EV42 IAkg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=jXZVJ+94XwHqxm8HHj5vG4KwS7WHyQS4F4Mb68xty3w=; b=TLv3J4/L8z3CG3ysp95oP97MZaOEkeBG0j0MjDX0nQPHYmtsGkTNuJAYDBwURHCv5+ /YDoejrn8jCnrKn+udSyeS+YYD0OeHN7Mk5FxRtFA0xQahNY/tm0h81D8SndVfW4KqFB EZiIhMMa9onN1AQ5v95uaobGF6gKXpawmJtKEaecvw52Qw/3bL8yZdEZ5ZjA3WxPYrvM 8l50soFfZkOEkG/RmvmXCJZINVFev5QGNS9D8p4f2t7SvJsYmLPFTDvSyTjZ7/6VuQtv q+FZEyNXTH9ywQRDmrA5saFm3tSOv24mQM/L3rGzquckPaJQ7M0pDlaxc/FaR1MLfxvf 5i9w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t6si2337365pfg.114.2018.04.27.17.59.58; Fri, 27 Apr 2018 18:00:12 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933227AbeD1A6e (ORCPT + 99 others); Fri, 27 Apr 2018 20:58:34 -0400 Received: from mail-ot0-f193.google.com ([74.125.82.193]:45615 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932851AbeD1A6c (ORCPT ); Fri, 27 Apr 2018 20:58:32 -0400 Received: by mail-ot0-f193.google.com with SMTP id w4-v6so3958017ote.12 for ; Fri, 27 Apr 2018 17:58: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:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jXZVJ+94XwHqxm8HHj5vG4KwS7WHyQS4F4Mb68xty3w=; b=YV9pylKMotoYbfnCF+J5XkL6htb3uqWv8avRmNNrADBJrxV4B2bQ/oVp2K2gZs0/tH iSKWyAiHUG4SR6j+pCynGkc/VyCY/h6qyspoS7Y59MQ6z5y/I1mOxx+Ult7y+5bQFUzd eU7Ug5VISGJ61QzriS9KRz42sBRcy3RU2AeJFStCLllk2NHRrUp8//EhT55yzRjSe5pm AdLzFYzzPXmWXEHOzUcDQ2PcaKswD0qx9sgWMgxIxbLH5+Gr2m+WoJt+7QGRbKh65E0J iq3zVzotYCYGvmUaeKDeISEzASXcflBS8ptAE3Uz7DOOprY4FaZxRCbAFav8EvIrfgOD xJHg== X-Gm-Message-State: ALQs6tC2RAbtJVUcpnJD9Iyjpio8llrbF8awIlJYrP3OztIr4BaOY25p VZBYz9lKC4Xl0+m1cRVNAXQp2g== X-Received: by 2002:a9d:3f49:: with SMTP id m67-v6mr3049538otc.133.1524877111866; Fri, 27 Apr 2018 17:58:31 -0700 (PDT) Received: from ?IPv6:2601:602:9802:a8dc::d2dd? ([2601:602:9802:a8dc::d2dd]) by smtp.gmail.com with ESMTPSA id 33-v6sm1457116oth.41.2018.04.27.17.58.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 27 Apr 2018 17:58:30 -0700 (PDT) Subject: Re: BUG: /proc/kcore does not export direct-mapped memory on arm64 (and presumably some other architectures) To: Kees Cook , Dave Anderson , Ard Biesheuvel Cc: Linux Kernel Mailing List , Ingo Molnar , Andi Kleen References: <981100282.24860394.1524770798522.JavaMail.zimbra@redhat.com> <823082096.24861749.1524771086176.JavaMail.zimbra@redhat.com> From: Laura Abbott Message-ID: <7ab806fe-ee36-59ad-483b-d6734fcd3451@redhat.com> Date: Fri, 27 Apr 2018 17:58:29 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/26/2018 02:16 PM, Kees Cook wrote: > On Thu, Apr 26, 2018 at 12:31 PM, Dave Anderson wrote: >> >> While testing /proc/kcore as the live memory source for the crash utility, >> it fails on arm64. The failure on arm64 occurs because only the >> vmalloc/module space segments are exported in PT_LOAD segments, >> and it's missing all of the PT_LOAD segments for the generic >> unity-mapped regions of physical memory, as well as their associated >> vmemmap sections. >> >> The mapping of unity-mapped RAM segments in fs/proc/kcore.c is >> architecture-neutral, and after debugging it, I found this as the >> problem. For each chunk of physical memory, kcore_update_ram() >> calls walk_system_ram_range(), passing kclist_add_private() as a >> callback function to add the chunk to the kclist, and eventually >> leading to the creation of a PT_LOAD segment. >> >> kclist_add_private() does some verification of the memory region, >> but this one below is bogus for arm64: >> >> static int >> kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg) >> { >> ... [ cut ] ... >> ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT)); >> ... [ cut ] ... >> >> /* Sanity check: Can happen in 32bit arch...maybe */ >> if (ent->addr < (unsigned long) __va(0)) >> goto free_out; >> >> And that's because __va(0) is a bogus check for arm64. It is checking >> whether the ent->addr value is less than the lowest possible unity-mapped >> address. But "0" should not be used as a physical address on arm64; the >> lowest legitimate physical address for this __va() check would be the arm64 >> PHYS_OFFSET, or memstart_addr: >> >> Here's the arm64 __va() and PHYS_OFFSET: >> >> #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) >> #define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET) >> >> extern s64 memstart_addr; >> /* PHYS_OFFSET - the physical address of the start of memory. */ >> #define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; }) >> >> If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000 on my >> test system), the __va(0) calculation goes negative and creates a bogus, very >> large, virtual address. And since the ent->addr virtual address is less than >> bogus __va(0) address, the test fails, and the memory chunk is rejected. >> >> Looking at the kernel sources, it seems that this would affect other >> architectures as well, i.e., the ones whose __va() is not a simple >> addition of the physical address with PAGE_OFFSET. >> >> Anyway, I don't know what the best approach for an architecture-neutral >> fix would be in this case. So I figured I'd throw it out to you guys for >> some ideas. > > I'm not as familiar with this code, but I've added Ard and Laura to CC > here, as this feels like something they'd be able to comment on. :) > > -Kees > It seems backwards that we're converting a physical address to a virtual address and then validating that. I think checking against pfn_valid (to ensure there is a valid memmap entry) and then checking page_to_virt against virt_addr_valid to catch other cases (e.g. highmem or holes in the space) seems cleaner. Maybe something like: diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c index d1e82761de81..e64ecb9f2720 100644 --- a/fs/proc/kcore.c +++ b/fs/proc/kcore.c @@ -209,25 +209,34 @@ kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg) { struct list_head *head = (struct list_head *)arg; struct kcore_list *ent; + struct page *p; + + if (!pfn_valid(pfn)) + return 1; + + p = pfn_to_page(pfn); + if (!memmap_valid_within(pfn, p, page_zone(p))) + return 1; ent = kmalloc(sizeof(*ent), GFP_KERNEL); if (!ent) return -ENOMEM; - ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT)); + ent->addr = (unsigned long)page_to_virt(p); ent->size = nr_pages << PAGE_SHIFT; - /* Sanity check: Can happen in 32bit arch...maybe */ - if (ent->addr < (unsigned long) __va(0)) + if (!virt_addr_valid(ent->addr)) goto free_out; /* cut not-mapped area. ....from ppc-32 code. */ if (ULONG_MAX - ent->addr < ent->size) ent->size = ULONG_MAX - ent->addr; - /* cut when vmalloc() area is higher than direct-map area */ - if (VMALLOC_START > (unsigned long)__va(0)) { - if (ent->addr > VMALLOC_START) - goto free_out; + /* + * We've already checked virt_addr_valid so we know this address + * is a valid pointer, therefore we can check against it to determine + * if we need to trim + */ + if (VMALLOC_START > ent->addr) { if (VMALLOC_START - ent->addr < ent->size) ent->size = VMALLOC_START - ent->addr; }