From patchwork Fri Jun 14 17:51:38 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Julien Grall X-Patchwork-Id: 10996151 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E835A14E5 for ; Fri, 14 Jun 2019 17:53:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DA08A286BC for ; Fri, 14 Jun 2019 17:53:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CEA4028720; Fri, 14 Jun 2019 17:53:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 388F328725 for ; Fri, 14 Jun 2019 17:53:27 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hbqMr-0001pH-0V; Fri, 14 Jun 2019 17:52:01 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hbqMo-0001nI-EA for xen-devel@lists.xenproject.org; Fri, 14 Jun 2019 17:51:58 +0000 X-Inumbo-ID: 1953a0f6-8ecd-11e9-939b-63e2fa18f954 Received: from foss.arm.com (unknown [217.140.110.172]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTP id 1953a0f6-8ecd-11e9-939b-63e2fa18f954; Fri, 14 Jun 2019 17:51:55 +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 75A492B; Fri, 14 Jun 2019 10:51:55 -0700 (PDT) Received: from e108454-lin.cambridge.arm.com (e108454-lin.cambridge.arm.com [10.1.196.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8DB523F718; Fri, 14 Jun 2019 10:51:54 -0700 (PDT) From: Julien Grall To: xen-devel@lists.xenproject.org Date: Fri, 14 Jun 2019 18:51:38 +0100 Message-Id: <20190614175144.20046-4-julien.grall@arm.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20190614175144.20046-1-julien.grall@arm.com> References: <20190614175144.20046-1-julien.grall@arm.com> Subject: [Xen-devel] [PATCH MM-PART3 v3 3/9] xen/arm: mm: Sanity check any update of Xen page tables X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Oleksandr_Tyshchenko@epam.com, Julien Grall , Stefano Stabellini , Andrii Anisov MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP The code handling Xen PT update has quite a few restrictions on what it can do. This is not a bad thing as it keeps the code simple. There are already a few checks scattered in current page table handling. However they are not sufficient as they could still allow to modify/remove entry with contiguous bit set. The checks are divided in two sets: - per entry check: They are gathered in a new function that will check whether an update is valid based on the flags passed and the current value of an entry. - global check: They are sanity check on xen_pt_update() parameters. Additionally to contiguous check, we also now check that the caller is not trying to modify the memory attributes of an entry. Lastly, it was probably a bit over the top to forbid removing an invalid mapping. This could just be ignored. The new behavior will be helpful in future changes. Signed-off-by: Julien Grall Reviewed-by: Andrii Anisov Reviewed-by: Stefano Stabellini --- Changes in v3: - Only allow modification on valid entry Changes in v2: - Correctly detect the removal of a page - Fix ASSERT on flags in the else case - Add Andrii's reviewed-by --- xen/arch/arm/mm.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 104 insertions(+), 18 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b13d9adf40..dcf041578b 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -50,6 +50,19 @@ #undef mfn_to_virt #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn)) +#ifdef NDEBUG +static inline void +__attribute__ ((__format__ (__printf__, 1, 2))) +mm_printk(const char *fmt, ...) {} +#else +#define mm_printk(fmt, args...) \ + do \ + { \ + dprintk(XENLOG_ERR, fmt, ## args); \ + WARN(); \ + } while (0); +#endif + #define DEFINE_PAGE_TABLES(name, nr) \ lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)] @@ -941,12 +954,81 @@ enum xenmap_operation { RESERVE }; +/* Sanity check of the entry */ +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags) +{ + /* Sanity check when modifying a page. */ + if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) ) + { + /* We don't allow modifying an invalid entry. */ + if ( !lpae_is_valid(entry) ) + { + mm_printk("Modifying invalid entry is not allowed.\n"); + return false; + } + + /* We don't allow changing memory attributes. */ + if ( entry.pt.ai != PAGE_AI_MASK(flags) ) + { + mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n", + entry.pt.ai, PAGE_AI_MASK(flags)); + return false; + } + + /* We don't allow modifying entry with contiguous bit set. */ + if ( entry.pt.contig ) + { + mm_printk("Modifying entry with contiguous bit set is not allowed.\n"); + return false; + } + } + /* Sanity check when inserting a page */ + else if ( flags & _PAGE_PRESENT ) + { + /* We should be here with a valid MFN. */ + ASSERT(!mfn_eq(mfn, INVALID_MFN)); + + /* We don't allow replacing any valid entry. */ + if ( lpae_is_valid(entry) ) + { + mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n", + mfn_x(lpae_get_mfn(entry)), mfn_x(mfn)); + return false; + } + } + /* Sanity check when removing a page. */ + else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 ) + { + /* We should be here with an invalid MFN. */ + ASSERT(mfn_eq(mfn, INVALID_MFN)); + + /* We don't allow removing page with contiguous bit set. */ + if ( entry.pt.contig ) + { + mm_printk("Removing entry with contiguous bit set is not allowed.\n"); + return false; + } + } + /* Sanity check when populating the page-table. No check so far. */ + else + { + ASSERT(flags & _PAGE_POPULATE); + /* We should be here with an invalid MFN */ + ASSERT(mfn_eq(mfn, INVALID_MFN)); + } + + return true; +} + static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr, mfn_t mfn, unsigned int flags) { lpae_t pte, *entry; lpae_t *third = NULL; + /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */ + ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT)); + entry = &xen_second[second_linear_offset(addr)]; if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) ) { @@ -962,15 +1044,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr, third = mfn_to_virt(lpae_get_mfn(*entry)); entry = &third[third_table_offset(addr)]; + if ( !xen_pt_check_entry(*entry, mfn, flags) ) + return -EINVAL; + switch ( op ) { case INSERT: case RESERVE: - if ( lpae_is_valid(*entry) ) - { - printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n", - __func__, addr, mfn_x(mfn)); - return -EINVAL; - } if ( op == RESERVE ) break; pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags)); @@ -982,12 +1061,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr, break; case MODIFY: case REMOVE: - if ( !lpae_is_valid(*entry) ) - { - printk("%s: trying to %s a non-existing mapping addr=%lx\n", - __func__, op == REMOVE ? "remove" : "modify", addr); - return -EINVAL; - } if ( op == REMOVE ) pte.bits = 0; else @@ -995,12 +1068,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr, pte = *entry; pte.pt.ro = PAGE_RO_MASK(flags); pte.pt.xn = PAGE_XN_MASK(flags); - if ( !pte.pt.ro && !pte.pt.xn ) - { - printk("%s: Incorrect combination for addr=%lx\n", - __func__, addr); - return -EINVAL; - } } write_pte(entry, pte); break; @@ -1022,6 +1089,25 @@ static int xen_pt_update(enum xenmap_operation op, int rc = 0; unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE; + /* + * The hardware was configured to forbid mapping both writeable and + * executable. + * When modifying/creating mapping (i.e _PAGE_PRESENT is set), + * prevent any update if this happen. + */ + if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) && + !PAGE_XN_MASK(flags) ) + { + mm_printk("Mappings should not be both Writeable and Executable.\n"); + return -EINVAL; + } + + if ( !IS_ALIGNED(virt, PAGE_SIZE) ) + { + mm_printk("The virtual address is not aligned to the page-size.\n"); + return -EINVAL; + } + spin_lock(&xen_pt_lock); for( ; addr < addr_end; addr += PAGE_SIZE )