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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8FC24C433EF for ; Fri, 19 Nov 2021 16:16:55 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 13E8961B1B for ; Fri, 19 Nov 2021 16:16:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 13E8961B1B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 857346B006C; Fri, 19 Nov 2021 11:16:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 804426B0071; Fri, 19 Nov 2021 11:16:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6A6936B0073; Fri, 19 Nov 2021 11:16:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0114.hostedemail.com [216.40.44.114]) by kanga.kvack.org (Postfix) with ESMTP id 5C2496B006C for ; Fri, 19 Nov 2021 11:16:44 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 1540989B5F for ; Fri, 19 Nov 2021 16:16:34 +0000 (UTC) X-FDA: 78826182708.13.F497CFB Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by imf28.hostedemail.com (Postfix) with ESMTP id C07F7900050D for ; Fri, 19 Nov 2021 16:16:33 +0000 (UTC) Received: by mail-lf1-f43.google.com with SMTP id l22so45389978lfg.7 for ; Fri, 19 Nov 2021 08:16:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PT99iXBgpFIqepfqqbxcYIJQ3M5siuSp1UK8d1YL76U=; b=jHnYvtbACgEqbUWh2jxPk+A/JrzDhbXD8GN8W5j+q116oOn4I3SOuYu6WXCtrrggre F7RpoUb/u9Up8UEnvaefDtRgviWR4D0TRw1rW/Phwc80x7ZOoDd8O2dfgi/c+ROR+lX5 Yy2fFiZqBJ5+RA1uTtjpnhhyj1hCKxOF02fIylR+5hy8M6tyWFndK21K7bQVKGshQUbZ i/FNRVp4LEQcjTlUBmMdV3GUS7OuM5jx1rMJe4MjQZPdej9ePypZgGW9aGrETXY4QiZp kerRyGX2XuCWsfQK8QYTwunTBoAEJdQvm1+FgZtHsirRQWxrNm3WV5XlSKSARBFjr8R5 GQrg== 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=PT99iXBgpFIqepfqqbxcYIJQ3M5siuSp1UK8d1YL76U=; b=48eCyn74eMlYLEVE2Xx/lkiMjpHrTPLcVpy/FwHiDd0a59STIP+5Xn+rtBfqYcg7Th 0M94Zv4FfuGZSkvK7CY5yN9pUtQKrJuwBOht1Qetv58iTL8NSpHZii9KuDJCUsM8dt/F Zo3ZTHCDavD8QdCSq8fcgimmv59USjkFWkFHt7sQtZFa91ojd44TycHyPhVPX+KcJP07 Q8u3Cqbd//YJhf/hZREhWby4MBKznruEvBAbz/xzvmOHQlxEmW8vYcEUiLycYBLDUqMp AZmHaR807PAZVJE0FbqY0I46kLYP1N2hGiB+tt6XYKMcsaJLrfKKOuBMzLHe+jTEIZ1W ClGw== X-Gm-Message-State: AOAM530WNf8KkspVo6Ko7v/Q4wEvALWweZT6+cteJFKMSvrdNMpxIlgg BqLC2QtsR/RBGUNGDmKjMyFOl90y+sK/rilXcfWt4g== X-Google-Smtp-Source: ABdhPJyWR8stzdTiCGjrREyonaNUfQgQWz4uaz8HWtQNIMD89ICA71wJAEljhEu8NgU+6QnOvVwM8eP3k2kVjE8qGLw= X-Received: by 2002:a2e:9f15:: with SMTP id u21mr26972655ljk.132.1637338591736; Fri, 19 Nov 2021 08:16:31 -0800 (PST) MIME-Version: 1.0 References: <20211110220731.2396491-1-brijesh.singh@amd.com> <20211110220731.2396491-44-brijesh.singh@amd.com> In-Reply-To: From: Peter Gonda Date: Fri, 19 Nov 2021 09:16:20 -0700 Message-ID: Subject: Re: [PATCH v7 43/45] virt: Add SEV-SNP guest driver To: Brijesh Singh Cc: x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , Joerg Roedel , Tom Lendacky , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , "Dr . David Alan Gilbert" , tony.luck@intel.com, marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: C07F7900050D X-Stat-Signature: 4ggjahp3p4jisx1w9rkmfado73tnqbo8 Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=jHnYvtbA; spf=pass (imf28.hostedemail.com: domain of pgonda@google.com designates 209.85.167.43 as permitted sender) smtp.mailfrom=pgonda@google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1637338593-691726 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 Thu, Nov 18, 2021 at 10:32 AM Brijesh Singh wrote: > > > > On 11/17/21 5:34 PM, Peter Gonda wrote: > > > >> +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device. > >> +The ioctl accepts struct snp_user_guest_request. The input and output structure is > >> +specified through the req_data and resp_data field respectively. If the ioctl fails > >> +to execute due to a firmware error, then fw_err code will be set. > > > > Should way say what it will be set to? Also Sean pointed out on CCP > > driver that 0 is strange to set the error to, its a uint so we cannot > > do -1 like we did there. What about all FFs? > > > > Sure, all FF's works, I can document and use it. > > > >> +static inline u64 __snp_get_msg_seqno(struct snp_guest_dev *snp_dev) > >> +{ > >> + u64 count; > > > > I may be overly paranoid here but how about > > `lockdep_assert_held(&snp_cmd_mutex);` when writing or reading > > directly from this data? > > > > Sure, I can do it. > > ... > > >> + > >> + if (rc) > >> + return rc; > >> + > >> + rc = verify_and_dec_payload(snp_dev, resp_buf, resp_sz); > >> + if (rc) { > >> + /* > >> + * The verify_and_dec_payload() will fail only if the hypervisor is > >> + * actively modifiying the message header or corrupting the encrypted payload. > > modifiying > >> + * This hints that hypervisor is acting in a bad faith. Disable the VMPCK so that > >> + * the key cannot be used for any communication. > >> + */ > > > > This looks great, thanks for changes Brijesh. Should we mention in > > comment here or at snp_disable_vmpck() the AES-GCM issues with > > continuing to use the key? Or will future updaters to this code > > understand already? > > > > Sure, I can add comment about the AES-GCM. > > ... > > >> + > >> +/* See SNP spec SNP_GUEST_REQUEST section for the structure */ > >> +enum msg_type { > >> + SNP_MSG_TYPE_INVALID = 0, > >> + SNP_MSG_CPUID_REQ, > >> + SNP_MSG_CPUID_RSP, > >> + SNP_MSG_KEY_REQ, > >> + SNP_MSG_KEY_RSP, > >> + SNP_MSG_REPORT_REQ, > >> + SNP_MSG_REPORT_RSP, > >> + SNP_MSG_EXPORT_REQ, > >> + SNP_MSG_EXPORT_RSP, > >> + SNP_MSG_IMPORT_REQ, > >> + SNP_MSG_IMPORT_RSP, > >> + SNP_MSG_ABSORB_REQ, > >> + SNP_MSG_ABSORB_RSP, > >> + SNP_MSG_VMRK_REQ, > >> + SNP_MSG_VMRK_RSP, > > > > Did you want to include MSG_ABSORB_NOMA_REQ and MSG_ABSORB_NOMA_RESP here? > > > > Yes, I can includes those for the completeness. > > ... > > >> +struct snp_report_req { > >> + /* message version number (must be non-zero) */ > >> + __u8 msg_version; > >> + > >> + /* user data that should be included in the report */ > >> + __u8 user_data[64]; > > > > Are we missing the 'vmpl' field here? Does those default all requests > > to be signed with VMPL0? Users might want to change that, they could > > be using a paravisor. > > > > Good question, so far I was thinking that guest kernel will provide its > vmpl level instead of accepted the vmpl level from the userspace. Do you > see a need for a userspace to provide this information ? That seems fine. I am just confused because we are just encrypting this struct as the payload for the PSP. Doesn't the message require a struct that looks like 'snp_report_req_user_data' below? snp_report_req{ /* message version number (must be non-zero) */ __u8 msg_version; /* user data that should be included in the report */ struct snp_report_req_user_data; }; struct snp_report_req_user_data { u8 user_data[64]; u32 vmpl; u32 reserved; }; > > > thanks