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=-23.2 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=unavailable 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 7005DC433DB for ; Wed, 27 Jan 2021 06:23:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1BF2B20679 for ; Wed, 27 Jan 2021 06:23:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234085AbhA0DNo (ORCPT ); Tue, 26 Jan 2021 22:13:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50980 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2395339AbhAZTPq (ORCPT ); Tue, 26 Jan 2021 14:15:46 -0500 Received: from mail-pg1-x52c.google.com (mail-pg1-x52c.google.com [IPv6:2607:f8b0:4864:20::52c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 37EAEC06174A for ; Tue, 26 Jan 2021 11:15:06 -0800 (PST) Received: by mail-pg1-x52c.google.com with SMTP id b21so5236675pgk.7 for ; Tue, 26 Jan 2021 11:15:06 -0800 (PST) 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=7PjVCLjYFXyjmxqV/lyzID0J0aEXBM2goYbT8N0cVdM=; b=N4qij7ElZXoW1edUjouKX7sa4B7PcH7S1ydi7zNInVxG98fDfQBwvWzAsPMCiXfAIi SwHvVjKK5tkLgarR+fR9Gr/pyisr1XQUoa32306zwNGoFGGjXd+a1wwkTsed9uIkigvU JheRiZjqYzetJS+Y60ZD6kh92Ot6U2lumGY28k1RZgy07h4/3lLjxckmNwnDprtZe5Td rjK75ljLmnhqu/oUJ5o4l4db8W6r2oRtZWSs+HHdKAVjLD0CadqLdY6D6X+rT3PnN4zj BxndHPd0RluJldv52zCGzT9lBalqFCFNWBB9pZW+YCYOqvaXK4mn+ScGpLrday1Oz8Dn obtQ== 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=7PjVCLjYFXyjmxqV/lyzID0J0aEXBM2goYbT8N0cVdM=; b=MVGmD1AXTvay1euyncTfNjrm2zc2VDK/HXxMrWrEp8+SXB0vQ8WnOk7OsPyPIEvquE TgCnGHqFPU2brzjEVKp+eLd4DnfXa+/7mWuf+LnCuu3NefzK3XLi6vCfRwJx7zqLLl05 MpvygDqMZ5e3VqGyE65I2z7/JXMpk8jWC5Z1liqYXYwbrTeDs2qw93oIH/g/llxCupqg ibTQVw0hkOvNewXowwwCIDpcl3g2Fq6dkXzaTm2FW6vZpQ8VzIPo1y0deLrw8ZKA5bB4 AmDdj/mDnCZG/c/i39TkOfL2Ud3NwdfezfKQVWe1pzMnaT/VlGqlDpxEy6WblseSUYpa LYkw== X-Gm-Message-State: AOAM530rodub2UAJLJZWxGvKdZtg4JD/fMZ7CL/exG1cbqLYDP4cMTL5 t7O8LW3DgMyp4iuSq9I0p1sCiQ== X-Google-Smtp-Source: ABdhPJywTF+70hdYYm6QZYKpIt1JIx4lGygSfSPQrTqWQ+lxlJJDsLxSag8n7PPPKhewpWJCupqstg== X-Received: by 2002:a05:6a00:16c7:b029:1b6:68a6:985a with SMTP id l7-20020a056a0016c7b02901b668a6985amr6557419pfc.44.1611688505551; Tue, 26 Jan 2021 11:15:05 -0800 (PST) Received: from google.com ([2620:15c:f:10:1ea0:b8ff:fe73:50f5]) by smtp.gmail.com with ESMTPSA id b65sm21356534pga.54.2021.01.26.11.15.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Jan 2021 11:15:04 -0800 (PST) Date: Tue, 26 Jan 2021 11:14:58 -0800 From: Sean Christopherson To: Peter Gonda Cc: kvm@vger.kernel.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Paolo Bonzini , Joerg Roedel , Tom Lendacky , Brijesh Singh , x86@kernel.org, stable@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] Fix unsynchronized access to sev members through svm_register_enc_region Message-ID: References: <20210126185431.1824530-1-pgonda@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210126185431.1824530-1-pgonda@google.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 26, 2021, Peter Gonda wrote: > sev_pin_memory assumes that callers hold the kvm->lock. This was true for > all callers except svm_register_enc_region since it does not originate This doesn't actually state what change it being made, is only describes the problem. I'd also reword these sentences to avoid talking about assumptions, and instead use stronger language. > from svm_mem_enc_op. Also added lockdep annotation to help prevent s/Also added/Add, i.e. describe what the patch is doing, not what you did in the past. E.g. Grab kvm->lock before pinning memory when registering an encrypted region; sev_pin_memory() relies on kvm->lock being held to ensure correctness when checking and updating the number of pinned pages. Add a lockdep assertion to help prevent future regressions. > future regressions. > > Tested: Booted SEV enabled VM on host. Personally I'd just leave this out. Unless stated otherwise, it's implied that you've tested the patch. > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Paolo Bonzini > Cc: Joerg Roedel > Cc: Tom Lendacky > Cc: Brijesh Singh > Cc: Sean Christopherson > Cc: x86@kernel.org > Cc: kvm@vger.kernel.org > Cc: stable@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Fixes: 116a2214c5173 (KVM: SVM: Pin guest memory when SEV is active) > Signed-off-by: Peter Gonda > > --- > arch/x86/kvm/svm.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index afdc5b44fe9f..9884e57f3d0f 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1699,6 +1699,8 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr, > struct page **pages; > unsigned long first, last; > > + lockdep_assert_held(&kvm->lock); > + > if (ulen == 0 || uaddr + ulen < uaddr) > return NULL; > > @@ -7228,12 +7230,19 @@ static int svm_register_enc_region(struct kvm *kvm, > if (!region) > return -ENOMEM; > > + mutex_lock(&kvm->lock); > region->pages = sev_pin_memory(kvm, range->addr, range->size, ®ion->npages, 1); > if (!region->pages) { > ret = -ENOMEM; > goto e_free; This error path needs to do mutex_unlock(). > } > > + region->uaddr = range->addr; > + region->size = range->size; > + > + list_add_tail(®ion->list, &sev->regions_list); > + mutex_unlock(&kvm->lock); > + > /* > * The guest may change the memory encryption attribute from C=0 -> C=1 > * or vice versa for this memory range. Lets make sure caches are > @@ -7242,13 +7251,6 @@ static int svm_register_enc_region(struct kvm *kvm, > */ > sev_clflush_pages(region->pages, region->npages); > > - region->uaddr = range->addr; > - region->size = range->size; > - > - mutex_lock(&kvm->lock); > - list_add_tail(®ion->list, &sev->regions_list); > - mutex_unlock(&kvm->lock); > - > return ret; > > e_free: > -- > 2.30.0.280.ga3ce27912f-goog