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 X-Spam-Level: X-Spam-Status: No, score=-1.9 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FSL_HELO_FAKE,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1746CC636C9 for ; Wed, 21 Jul 2021 13:35:41 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 7B50961241 for ; Wed, 21 Jul 2021 13:35:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7B50961241 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id ED1D84B110; Wed, 21 Jul 2021 09:35:39 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tNKS947EZY5e; Wed, 21 Jul 2021 09:35:38 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id BF4184B0E9; Wed, 21 Jul 2021 09:35:38 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 1D8B74B0E9 for ; Wed, 21 Jul 2021 09:35:37 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ff7BOz-lUmU0 for ; Wed, 21 Jul 2021 09:35:35 -0400 (EDT) Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id C59454A483 for ; Wed, 21 Jul 2021 09:35:35 -0400 (EDT) Received: by mail-wm1-f47.google.com with SMTP id f190so1338261wmf.4 for ; Wed, 21 Jul 2021 06:35:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=dYzzqSbTcHoAKX52adHhFah33UAvJMaxB1AIimYVp8/ShQNJyvzpQuyW4v9H9wQS7C 9oUflpJc11rRJeujwNJ/0OaIN7oOBEwVF7hMZpgJorzEtN81xw7UznEeWpFXOCMU+9Yf vvlCvBwEaXtKD+Mit2oA1qgc2x2cQOCvdQD8ad6Jmyx3IV+U1K1ht0zp7+FRoLComa9z 7nyg//dnCxl4r1LGd2OmqdUp+AIhctUAyBKbvsw/8+fgpsxlsGVuOTjFWbitTHU3+qua iXTTZxxsP4ssGpvdcORzQIsJquYp1mYs6zK7WvpK/a82z1I5x6mBKwGwDf4o5qiqrpRR CN8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=Rp9p0tw8D9vr5jYO4pEhqUuesQQvJwIKOKNlWrA98MX+nzNiEBdEqzb1gu3pZpehzN +rDIQ4Nq1B97IV7Gl+KpMUqWUy06+N6t+P5Ior7nPclkJrMPQc38z5ct1Swcm5BHl/Wm +ndICox+bm/bmjtuQUf9BAvF12Ah5oVTemG2aRSD3KNPkMnoXtAr6++sczP3L7al73xf 2VikgLmn/tB3CJC1bWoPeAm2NtjNZjxM95IFUg9TXfghjknzUEHs3rhq+pozft49QvHY YEc82wI+Fgj3RxxYdrO9gcpcL1aVZ4p0K43lUlGqMmViEZcukIiEk5714gRWY9JyCjGG rfrg== X-Gm-Message-State: AOAM532PvsI3hy94/4uSLpfN9D+3gtPL2uawrcJIQowRfKxHgquXp4lR Jks3fPtfGExfMKClD6eiJMjHyg== X-Google-Smtp-Source: ABdhPJyciOXfOFa9KQ+iYzD+QsJY74g96fGc86hUOZHCThYzhTYjYByChhSiI5DH0qDjIx7s8PRrdA== X-Received: by 2002:a1c:1bc3:: with SMTP id b186mr4197684wmb.27.1626874534544; Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:efb1:2fcc:e84:52ad]) by smtp.gmail.com with ESMTPSA id e11sm33189602wrt.0.2021.07.21.06.35.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Date: Wed, 21 Jul 2021 14:35:30 +0100 From: Quentin Perret To: Fuad Tabba Subject: Re: [PATCH 13/14] KVM: arm64: Restrict hyp stage-1 manipulation in protected mode Message-ID: References: <20210719104735.3681732-1-qperret@google.com> <20210719104735.3681732-14-qperret@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Cc: kernel-team@android.com, qwandor@google.com, maz@kernel.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Fuad, On Wednesday 21 Jul 2021 at 11:45:53 (+0100), Fuad Tabba wrote: > > +static int hyp_range_is_shared_walker(u64 addr, u64 end, u32 level, > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + if (!kvm_pte_valid(pte)) > > + return -EPERM; > > + > > + prot = kvm_pgtable_hyp_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > nit: is this check necessary? I guess not, the next one should do, I'll remove :) > > + /* Check that the page has been shared with the hypervisor before */ > > + if (prot != (PAGE_HYP | KVM_PGTABLE_STATE_SHARED | KVM_PGTABLE_STATE_BORROWED)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > +static int hyp_range_is_shared(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = hyp_range_is_shared_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&pkvm_pgtable, (u64)__hyp_va(start), > > + end - start, &walker); > > +} > > + > > +static int check_host_share_hyp_walker(u64 addr, u64 end, u32 level, > > nit: It seems the convention is usually addr,size or start,end. Here > you're using addr,end. Well for some reason all the walkers in pgtable.c follow the addr,end convention, so I followed that. But in fact, as per [1] I might actually get rid of this walker in v2, so hopefully that'll make the issue go away. [1] https://lore.kernel.org/kvmarm/YPbwmVk1YD9+y7tr@google.com/ > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + /* If invalid, only allow to share pristine pages */ > > + if (!kvm_pte_valid(pte)) > > + return pte ? -EPERM : 0; > > + > > + prot = kvm_pgtable_stage2_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > > + > > + /* Cannot share a page that is not owned */ > > + if (prot & KVM_PGTABLE_STATE_BORROWED) > > + return -EPERM; > > + > > + /* Cannot share a page with restricted access */ > > + if ((prot & KVM_PGTABLE_PROT_RWX) ^ KVM_PGTABLE_PROT_RWX) > nit: isn't this clearer as > > if ((prot & KVM_PGTABLE_PROT_RWX) != KVM_PGTABLE_PROT_RWX) I guess it would be, I'll fix it up. > > + return -EPERM; > > + > > + /* Allow double-sharing (requires cross-checking the hyp stage-1) */ > > + if (prot & KVM_PGTABLE_STATE_SHARED) > > + return hyp_range_is_shared(addr, addr + 1); > > Why addr+1, rather than end? Because 'end' here is the 'end' that was passed to kvm_pgtable_walk() IIRC. What I want to do here is check if the page I'm currently visiting is already shared and if so, that it is shared with the hypervisor. But it's possible that only one page in the range of pages passed to __pkvm_host_share_hyp is already shared, so I need to check them one by one. Anyways, as per the discussion with Marc on [2], I'll probably switch to only accept sharing one page at a time, so all these issues should just go away as well! [2] https://lore.kernel.org/kvmarm/YPa6BGuUFjw8do+o@google.com/ > > + > > + return 0; > > +} > > + > > +static int check_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = check_host_share_hyp_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&host_kvm.pgt, start, end - start, &walker); > > +} > > + > > +int __pkvm_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + enum kvm_pgtable_prot prot; > > + int ret; > > + > > + if (!range_is_memory(start, end)) > > + return -EINVAL; > > + > > + hyp_spin_lock(&host_kvm.lock); > > + hyp_spin_lock(&pkvm_pgd_lock); > > + > > + ret = check_host_share_hyp(start, end); > > + if (ret) > > + goto unlock; > > + > > + prot = KVM_PGTABLE_PROT_RWX | KVM_PGTABLE_STATE_SHARED; > > + ret = host_stage2_idmap_locked(start, end, prot); > > Just for me to understand this better. The id mapping here, which > wasn't taking place before this patch, is for tracking, right? Yes, exactly, I want to make sure to mark the page as shared (and borrowed) in the relevant page-tables to not forget about it :) Cheers, Quentin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E3A6DC12002 for ; Wed, 21 Jul 2021 13:37:04 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AFB8F60FEA for ; Wed, 21 Jul 2021 13:37:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AFB8F60FEA Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tT7jSGzUwlVYlqrOSQVms4JmER0PedC7YIY313q+jLY=; b=Vtu5lmJ+RsuOcc 2tMQxaLNnUe3D0Mk4kfZ7HKDF29MG9Rcg/u4pIf2Zp27QxlA1RHTBx1QpM8Gt311qJMOmvbCS3CcA 57hOlC8BR26q1/CS6yiZmkjjy1ctcGpxZTJWWYW0YupO209iUeKNcZR6e4t+5lphMmqhxbs9PeVgQ salT//8Xi+R8zHclT3ExlpViUKG918YBOW9QqQ9LBTm+nwo0yrgOjbxR25/1jHpgP466lVwZ/WXbx l8gC4BGXZjKI9IzbmiF7oXGqvycEaD4wapjyfxo4HoeRYGJpR0Yt2kaWyJ6JxKgeCq/n3fO7DcNm2 VewakLIw5gzwzYqA0BWQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m6CNv-00FlTF-HQ; Wed, 21 Jul 2021 13:35:39 +0000 Received: from mail-wm1-x333.google.com ([2a00:1450:4864:20::333]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1m6CNs-00FlSg-8p for linux-arm-kernel@lists.infradead.org; Wed, 21 Jul 2021 13:35:38 +0000 Received: by mail-wm1-x333.google.com with SMTP id l6so1372089wmq.0 for ; Wed, 21 Jul 2021 06:35:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=dYzzqSbTcHoAKX52adHhFah33UAvJMaxB1AIimYVp8/ShQNJyvzpQuyW4v9H9wQS7C 9oUflpJc11rRJeujwNJ/0OaIN7oOBEwVF7hMZpgJorzEtN81xw7UznEeWpFXOCMU+9Yf vvlCvBwEaXtKD+Mit2oA1qgc2x2cQOCvdQD8ad6Jmyx3IV+U1K1ht0zp7+FRoLComa9z 7nyg//dnCxl4r1LGd2OmqdUp+AIhctUAyBKbvsw/8+fgpsxlsGVuOTjFWbitTHU3+qua iXTTZxxsP4ssGpvdcORzQIsJquYp1mYs6zK7WvpK/a82z1I5x6mBKwGwDf4o5qiqrpRR CN8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=hdF3SJUMy62gV4rt70jZfS4K410+Bc+/CMLnnSvgJXEWRgHtF0vQdBJiCdPSeqjBnI nYC+jDWk2DjM27OPvVtXDyuNOoA0C9j8cHs41rjXQahBpKKlLxZoFvCzsIJoDD7yjreJ kIFeNaXcPU6MNMdQBcJLXGSffa0bafFM4jcjHIjvwogYJ+5FnfYVbENZcV1iJ5emFBnT ENMeukC9WH6xFsWusGzfMvxNIvxlYP3PHfTOEosEgjSMIKx13B5cG1RPULgHbveZv3/4 1jZDJpCW/oWHXi91v6fByN5FEYFz7PYdxcWkJi+EOMdEJa7veBMQt+VerHF9C+l52jnd rugA== X-Gm-Message-State: AOAM530GERQY8ecpdIcGIDFyDInAcvWPsZ2PPAMSsce6X9oNxpHlvgR0 VTfsRRTxt6EcuOHkp8ouP5/8xA== X-Google-Smtp-Source: ABdhPJyciOXfOFa9KQ+iYzD+QsJY74g96fGc86hUOZHCThYzhTYjYByChhSiI5DH0qDjIx7s8PRrdA== X-Received: by 2002:a1c:1bc3:: with SMTP id b186mr4197684wmb.27.1626874534544; Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:efb1:2fcc:e84:52ad]) by smtp.gmail.com with ESMTPSA id e11sm33189602wrt.0.2021.07.21.06.35.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Date: Wed, 21 Jul 2021 14:35:30 +0100 From: Quentin Perret To: Fuad Tabba Cc: maz@kernel.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, ardb@kernel.org, qwandor@google.com, dbrazdil@google.com, kernel-team@android.com Subject: Re: [PATCH 13/14] KVM: arm64: Restrict hyp stage-1 manipulation in protected mode Message-ID: References: <20210719104735.3681732-1-qperret@google.com> <20210719104735.3681732-14-qperret@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210721_063536_379928_482CB94F X-CRM114-Status: GOOD ( 26.94 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Fuad, On Wednesday 21 Jul 2021 at 11:45:53 (+0100), Fuad Tabba wrote: > > +static int hyp_range_is_shared_walker(u64 addr, u64 end, u32 level, > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + if (!kvm_pte_valid(pte)) > > + return -EPERM; > > + > > + prot = kvm_pgtable_hyp_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > nit: is this check necessary? I guess not, the next one should do, I'll remove :) > > + /* Check that the page has been shared with the hypervisor before */ > > + if (prot != (PAGE_HYP | KVM_PGTABLE_STATE_SHARED | KVM_PGTABLE_STATE_BORROWED)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > +static int hyp_range_is_shared(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = hyp_range_is_shared_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&pkvm_pgtable, (u64)__hyp_va(start), > > + end - start, &walker); > > +} > > + > > +static int check_host_share_hyp_walker(u64 addr, u64 end, u32 level, > > nit: It seems the convention is usually addr,size or start,end. Here > you're using addr,end. Well for some reason all the walkers in pgtable.c follow the addr,end convention, so I followed that. But in fact, as per [1] I might actually get rid of this walker in v2, so hopefully that'll make the issue go away. [1] https://lore.kernel.org/kvmarm/YPbwmVk1YD9+y7tr@google.com/ > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + /* If invalid, only allow to share pristine pages */ > > + if (!kvm_pte_valid(pte)) > > + return pte ? -EPERM : 0; > > + > > + prot = kvm_pgtable_stage2_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > > + > > + /* Cannot share a page that is not owned */ > > + if (prot & KVM_PGTABLE_STATE_BORROWED) > > + return -EPERM; > > + > > + /* Cannot share a page with restricted access */ > > + if ((prot & KVM_PGTABLE_PROT_RWX) ^ KVM_PGTABLE_PROT_RWX) > nit: isn't this clearer as > > if ((prot & KVM_PGTABLE_PROT_RWX) != KVM_PGTABLE_PROT_RWX) I guess it would be, I'll fix it up. > > + return -EPERM; > > + > > + /* Allow double-sharing (requires cross-checking the hyp stage-1) */ > > + if (prot & KVM_PGTABLE_STATE_SHARED) > > + return hyp_range_is_shared(addr, addr + 1); > > Why addr+1, rather than end? Because 'end' here is the 'end' that was passed to kvm_pgtable_walk() IIRC. What I want to do here is check if the page I'm currently visiting is already shared and if so, that it is shared with the hypervisor. But it's possible that only one page in the range of pages passed to __pkvm_host_share_hyp is already shared, so I need to check them one by one. Anyways, as per the discussion with Marc on [2], I'll probably switch to only accept sharing one page at a time, so all these issues should just go away as well! [2] https://lore.kernel.org/kvmarm/YPa6BGuUFjw8do+o@google.com/ > > + > > + return 0; > > +} > > + > > +static int check_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = check_host_share_hyp_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&host_kvm.pgt, start, end - start, &walker); > > +} > > + > > +int __pkvm_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + enum kvm_pgtable_prot prot; > > + int ret; > > + > > + if (!range_is_memory(start, end)) > > + return -EINVAL; > > + > > + hyp_spin_lock(&host_kvm.lock); > > + hyp_spin_lock(&pkvm_pgd_lock); > > + > > + ret = check_host_share_hyp(start, end); > > + if (ret) > > + goto unlock; > > + > > + prot = KVM_PGTABLE_PROT_RWX | KVM_PGTABLE_STATE_SHARED; > > + ret = host_stage2_idmap_locked(start, end, prot); > > Just for me to understand this better. The id mapping here, which > wasn't taking place before this patch, is for tracking, right? Yes, exactly, I want to make sure to mark the page as shared (and borrowed) in the relevant page-tables to not forget about it :) Cheers, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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 X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A9CCC636CE for ; Wed, 21 Jul 2021 13:38:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D5FBF6120E for ; Wed, 21 Jul 2021 13:38:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238087AbhGUM5X (ORCPT ); Wed, 21 Jul 2021 08:57:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43162 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238176AbhGUMzv (ORCPT ); Wed, 21 Jul 2021 08:55:51 -0400 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 47F11C061574 for ; Wed, 21 Jul 2021 06:35:36 -0700 (PDT) Received: by mail-wm1-x332.google.com with SMTP id u8-20020a7bcb080000b02901e44e9caa2aso998608wmj.4 for ; Wed, 21 Jul 2021 06:35:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=dYzzqSbTcHoAKX52adHhFah33UAvJMaxB1AIimYVp8/ShQNJyvzpQuyW4v9H9wQS7C 9oUflpJc11rRJeujwNJ/0OaIN7oOBEwVF7hMZpgJorzEtN81xw7UznEeWpFXOCMU+9Yf vvlCvBwEaXtKD+Mit2oA1qgc2x2cQOCvdQD8ad6Jmyx3IV+U1K1ht0zp7+FRoLComa9z 7nyg//dnCxl4r1LGd2OmqdUp+AIhctUAyBKbvsw/8+fgpsxlsGVuOTjFWbitTHU3+qua iXTTZxxsP4ssGpvdcORzQIsJquYp1mYs6zK7WvpK/a82z1I5x6mBKwGwDf4o5qiqrpRR CN8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=THtvL95+Wdg5XKa0Q6wjHIENl0Y9TZHt3CwdcuBkgF8=; b=JzKW+kpNgaZ4XGKuRNR7rH0Nt+z7NaI7MMZs8N0Zs4v1+2M/gRBX3LQZv7a98yZ72b gMJFH2nrcPeAmS4955mi0Uoi/cJTdxKTxrC+wzhj/bA5feCF6BgY1/AiLXgGRY+fISUW o6whZgLLXvq5iKixL++0qp0/nc+r2qJ7+r27fax65zXDZSI6NXYFnsQCb51CXI2jtnRj h5wKy0vq+MGTDk4/dgl8rmBEbpyO2xQkjN+Obo3GxWXNiuoDCKc2kTEQI9URgZl7uFOY wbtserPtIRFEpdYV1t8cW2Nux1+vZFcBIR27eymXzX9cZghk7QJbYG/qVV80z77Z202y WELA== X-Gm-Message-State: AOAM530oceHcWFn2+7GiGJ3Iop3zXeuZDB3N+1TajA8qIgu67N5tBfap dvPHubvvxuW67IMaBUshLoRxwQ== X-Google-Smtp-Source: ABdhPJyciOXfOFa9KQ+iYzD+QsJY74g96fGc86hUOZHCThYzhTYjYByChhSiI5DH0qDjIx7s8PRrdA== X-Received: by 2002:a1c:1bc3:: with SMTP id b186mr4197684wmb.27.1626874534544; Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:efb1:2fcc:e84:52ad]) by smtp.gmail.com with ESMTPSA id e11sm33189602wrt.0.2021.07.21.06.35.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Jul 2021 06:35:34 -0700 (PDT) Date: Wed, 21 Jul 2021 14:35:30 +0100 From: Quentin Perret To: Fuad Tabba Cc: maz@kernel.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, ardb@kernel.org, qwandor@google.com, dbrazdil@google.com, kernel-team@android.com Subject: Re: [PATCH 13/14] KVM: arm64: Restrict hyp stage-1 manipulation in protected mode Message-ID: References: <20210719104735.3681732-1-qperret@google.com> <20210719104735.3681732-14-qperret@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Fuad, On Wednesday 21 Jul 2021 at 11:45:53 (+0100), Fuad Tabba wrote: > > +static int hyp_range_is_shared_walker(u64 addr, u64 end, u32 level, > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + if (!kvm_pte_valid(pte)) > > + return -EPERM; > > + > > + prot = kvm_pgtable_hyp_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > nit: is this check necessary? I guess not, the next one should do, I'll remove :) > > + /* Check that the page has been shared with the hypervisor before */ > > + if (prot != (PAGE_HYP | KVM_PGTABLE_STATE_SHARED | KVM_PGTABLE_STATE_BORROWED)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > +static int hyp_range_is_shared(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = hyp_range_is_shared_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&pkvm_pgtable, (u64)__hyp_va(start), > > + end - start, &walker); > > +} > > + > > +static int check_host_share_hyp_walker(u64 addr, u64 end, u32 level, > > nit: It seems the convention is usually addr,size or start,end. Here > you're using addr,end. Well for some reason all the walkers in pgtable.c follow the addr,end convention, so I followed that. But in fact, as per [1] I might actually get rid of this walker in v2, so hopefully that'll make the issue go away. [1] https://lore.kernel.org/kvmarm/YPbwmVk1YD9+y7tr@google.com/ > > + kvm_pte_t *ptep, > > + enum kvm_pgtable_walk_flags flag, > > + void * const arg) > > +{ > > + enum kvm_pgtable_prot prot; > > + kvm_pte_t pte = *ptep; > > + > > + /* If invalid, only allow to share pristine pages */ > > + if (!kvm_pte_valid(pte)) > > + return pte ? -EPERM : 0; > > + > > + prot = kvm_pgtable_stage2_pte_prot(pte); > > + if (!prot) > > + return -EPERM; > > + > > + /* Cannot share a page that is not owned */ > > + if (prot & KVM_PGTABLE_STATE_BORROWED) > > + return -EPERM; > > + > > + /* Cannot share a page with restricted access */ > > + if ((prot & KVM_PGTABLE_PROT_RWX) ^ KVM_PGTABLE_PROT_RWX) > nit: isn't this clearer as > > if ((prot & KVM_PGTABLE_PROT_RWX) != KVM_PGTABLE_PROT_RWX) I guess it would be, I'll fix it up. > > + return -EPERM; > > + > > + /* Allow double-sharing (requires cross-checking the hyp stage-1) */ > > + if (prot & KVM_PGTABLE_STATE_SHARED) > > + return hyp_range_is_shared(addr, addr + 1); > > Why addr+1, rather than end? Because 'end' here is the 'end' that was passed to kvm_pgtable_walk() IIRC. What I want to do here is check if the page I'm currently visiting is already shared and if so, that it is shared with the hypervisor. But it's possible that only one page in the range of pages passed to __pkvm_host_share_hyp is already shared, so I need to check them one by one. Anyways, as per the discussion with Marc on [2], I'll probably switch to only accept sharing one page at a time, so all these issues should just go away as well! [2] https://lore.kernel.org/kvmarm/YPa6BGuUFjw8do+o@google.com/ > > + > > + return 0; > > +} > > + > > +static int check_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + struct kvm_pgtable_walker walker = { > > + .cb = check_host_share_hyp_walker, > > + .flags = KVM_PGTABLE_WALK_LEAF, > > + }; > > + > > + return kvm_pgtable_walk(&host_kvm.pgt, start, end - start, &walker); > > +} > > + > > +int __pkvm_host_share_hyp(phys_addr_t start, phys_addr_t end) > > +{ > > + enum kvm_pgtable_prot prot; > > + int ret; > > + > > + if (!range_is_memory(start, end)) > > + return -EINVAL; > > + > > + hyp_spin_lock(&host_kvm.lock); > > + hyp_spin_lock(&pkvm_pgd_lock); > > + > > + ret = check_host_share_hyp(start, end); > > + if (ret) > > + goto unlock; > > + > > + prot = KVM_PGTABLE_PROT_RWX | KVM_PGTABLE_STATE_SHARED; > > + ret = host_stage2_idmap_locked(start, end, prot); > > Just for me to understand this better. The id mapping here, which > wasn't taking place before this patch, is for tracking, right? Yes, exactly, I want to make sure to mark the page as shared (and borrowed) in the relevant page-tables to not forget about it :) Cheers, Quentin