Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 11:05:17 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Peter Gonda <pgonda@google.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     <dhowells@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Dionna Glaze <dionnaglaze@google.com>,
        "Brijesh Singh" <brijesh.singh@amd.com>, <peterz@infradead.org>,
        <linux-coco@lists.linux.dev>, <keyrings@vger.kernel.org>,
        <x86@kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] virt: sevguest: Add TSM key support for SNP_{GET,
 GET_EXT}_REPORT
Message-ID: <64c7f7ddd777c_51ad029436@dwillia2-xfh.jf.intel.com.notmuch>
References: <169057265210.180586.7950140104251236598.stgit@dwillia2-xfh.jf.intel.com>
 <169057267580.180586.15710177655506555147.stgit@dwillia2-xfh.jf.intel.com>
 <CAMkAt6r=r_utT6sd_LD-2rO-GpH4zd1D04p04P8WF51BKX-JMg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMkAt6r=r_utT6sd_LD-2rO-GpH4zd1D04p04P8WF51BKX-JMg@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UVNteUpOaCsrOXUreWd5dFNRRDkxQUVFRGVKSnMwRmZ2R1V4SjFVaURHVFN3?=
 =?utf-8?B?RFJ0amVPN0IwKzBxYzIwbjd4R3RTZTlvMmxCZXp0RDdaY3RueXBFNHEycUlP?=
 =?utf-8?B?VGxCK20wRUluMDNmSU5YYmdRblZNTW1Zb2JoSTBFVUV5WWRubUVveGNESDgv?=
 =?utf-8?B?MzVoSmdnb0czM1MySTFlNS9KczZmaHVXci8rbEhrVVFJMlZDbXdCM3ZVUnVG?=
 =?utf-8?B?d25UY1R1NVg1cnFKcHMvVDNzQUhYUmFwM2hSVkYrVmN3TjRQcWxEOGU1ZE5t?=
 =?utf-8?B?RlBmbUh4T2k3dXh5Mjhlbzc3VldTQjlNdm16NTZUU0ZpWmpSZ0NnWS80TXYz?=
 =?utf-8?B?RzArMFMwN3lScVE5OW1RQzJhUENES0FBakp6eVYrazVhTU1kT0tsRHI4ZC9C?=
 =?utf-8?B?TURZRmUzbjkyazhmZW81a2Y5dGxtdHRJWGl2aTVHcUxzaWpzSFV0amZ0MGtS?=
 =?utf-8?B?YVpQUUxmQ2FHZnMrZGVSS3ZxRS85UWtUaml0WUQxeG9XcEJ2dlBmMzRGUkpi?=
 =?utf-8?B?N2t3TCtlR2dCbGEyMDVnYnN4SXo4Y3VjZFIwYXVDWFoyVmViZUdjajNxK1R0?=
 =?utf-8?B?MUJORElVNVVHeUlYamNBUXhzVzZDUnV3YzNydkRra2pUT0xWOHIrYk9Oc24y?=
 =?utf-8?B?aFhGZk9CRUcvSzhJQzIyLzE0YzVrb1NOenB1WTRrVThaNDZ3d2x2NG1qME9p?=
 =?utf-8?B?alZTRTg3NEJhcjVneWFqeXhBRTBBNUVQV0Nhd05QclRvcnB2UEZ2NzkydmpL?=
 =?utf-8?B?amQvTElKOFhPN1EwWEtpZ3JSbjJYVGRLVnZPZ0oxazA0bFB1aGRPaXJPSGgv?=
 =?utf-8?B?bXdxMWV5dUZkUm1nQnJNMVMvRTlMNnp2a1JNOWMrcU5Ka0ZxandvT2RSMmRt?=
 =?utf-8?B?Sm5DMXg4RmpmUCtnNERLaTFrLzR3UmRtRFFCZUY5elkybXNUb3VaZk15NmJS?=
 =?utf-8?B?anl5cGJhMjJFaTc4emdZOE1xcU5nUmg0NlNNbm1WZDhNck1QcHNFSGxKZW80?=
 =?utf-8?B?TlBJQXdjZG40MncyMEN4N3RYa3Fkdmxqd2ZiRGhnaisyZ2J6ZXVxQzRScWQ0?=
 =?utf-8?B?YXdwTGxlaU5INGcwTVJtUnY3cG43TUR3am9PVEZORGt2YUNHWDBZZkJJbCtt?=
 =?utf-8?B?S2QxZldPRUVxc21rZU5hd2FXNy95bmwyKzd2Sy8vc3hTdHV2bVUzM0V4SXV1?=
 =?utf-8?B?bmdoNEpoTW8vVWd1ZXdEZnBnbnZJc0xnUVNiSmZqTCtWdXl1ZFZQMTF3aDNK?=
 =?utf-8?B?ZTBFTEFvOWhOdnFOTTFnY0JodUZXeTRoOThWTFFYSDFpWDdmUGNvUG9xcWZu?=
 =?utf-8?B?YzR0Y1hqQzQ1SlFXTHlOZlhXWWlSTmhYRlZrQk9jZGZIc0FuQitEYk9MVXI2?=
 =?utf-8?B?K2s5UXVyRGttK2FFRWE3K0R6N0RNRXFWTi9tKzU5L3k4WDJLMkxrOUl0RDQ4?=
 =?utf-8?B?Yzl4cVgydTg1N0hYeC9uYVN6M0FsL1VrSGJSOVM4dlhWbTJlUnBpMVV1R2t1?=
 =?utf-8?B?blEreTVrU05YL2pQZnp5bnpkQTV4MktKYXpQanNPb0xkVGJDa3BUajRZNjVa?=
 =?utf-8?B?UXZ2dVU1UytNVkVoUlR2eTlvS3BBVlJvYnBhNHNTekVNUnQrd1ExZzltL2Vx?=
 =?utf-8?B?V0dxN0xQTGwyNSt6UkpXOUVxRC9PbC9yeDBJVjVjbHRvek5ZWmRZejM3ZGxJ?=
 =?utf-8?B?cVBHU1NTbWZ0QXoxNVdsQWU0L0JLajJEc21Gb3M1ZVc0T2lHUnlyRnNRR0pB?=
 =?utf-8?B?S216bVhJR0Nya0pkOWpsb2pDUnY4azByY0p6YklUL0swMVkwVkx4MUlVaDh6?=
 =?utf-8?B?NHJ6TTF5Z0pHL0xhUnp0aE9UY1djZ1hsTVBUcWlLZHVVZ0lWcTdTWURtdlRF?=
 =?utf-8?B?UVUxWkZVMkdqd2pWdjNOVzBteExTck5nZEs1Q0VkNGpEWnBueFJMOWl1Rkg2?=
 =?utf-8?B?MGRvQ1RBaHpUeFJFbkZWcXZ6WCtXT05oSmpBdm15UDFMNFovNkg5aUswRVZC?=
 =?utf-8?B?bHlPWDYvaFlVbVBDck9BcVQ5ZncxdFBHZmhjK0RBdXdCdnZsaXFJSE80Tlh0?=
 =?utf-8?B?YXVkVVhQOVVWdi9OL282cmlhcGg3UjN1QXpSdGdNY3ZscTlidzludFoyZFRJ?=
 =?utf-8?B?Wi9uV2ZhTDU2VTZEbnY3dkRjTW5LaFoxSzlqdGJ0NEJxTHRQSE11bCtkTndr?=
 =?utf-8?B?Wmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8935d872-2908-4965-7146-08db91f0b4c6
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jul 2023 18:05:21.5470
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hs/04jA58oEgij+4BbFt/2zP2Goo8eX46kSgURPBXFxGXORWDlQ+73BYjUkMdLMVe2iMvQyXFqmsFyWyroplj+1ME3NkDu2d7r8jOOlHan4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8154
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Peter Gonda wrote:
> On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > The sevguest driver was a first mover in the confidential computing
> > space. As a first mover that afforded some leeway to build the driver
> > without concern for common infrastructure.
> >
> > Now that sevguest is no longer a singleton [1] the common operation of
> > building and transmitting attestation report blobs can / should be made
> > common. In this model the so called "TSM-provider" implementations can
> > share a common envelope ABI even if the contents of that envelope remain
> > vendor-specific. When / if the industry agrees on an attestation record
> > format, that definition can also fit in the same ABI. In the meantime
> > the kernel's maintenance burden is reduced and collaboration on the
> > commons is increased.
> >
> > Convert sevguest to use TSM keys to retrieve the blobs that the
> > SNP_{GET,GET_EXT}_REPORT ioctls produce. The flow for retrieving the
> > SNP_GET_REPORT blob via the keyctl utility would be:
> >
> >     dd if=/dev/urandom of=pubkey bs=1 count=64
> >     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u
> >     keyctl print $key_id | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> >
> > ...while the SNP_GET_EXT_REPORT flow adds the "format=extended" option
> > to the request flow:
> >
> >     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2 format=extended" @u
> >
> > The output format from 'keyctl print' is:
> >
> >     <pubkey blob> <auth blob desc[:format]> <auth blob>
> >
> > ...where the blobs are hex encoded and the descriptor string is either
> > "sev" or "sev:extended" in this case.
> >
> > Note, the Keys subsystem frontend for the functionality that
> > SNP_GET_DERIVED_KEY represents is saved for follow-on work that likely
> > needs to become a new trusted-keys type. The old ioctls can be lazily
> > deprecated, the main motivation of this effort is to stop the
> > proliferation of new ioctls, and to increase cross-vendor colloboration.
> 
> collaboration

got it.

> 
> >
> > Note, only compile-tested.
> >
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/virt/coco/sev-guest/Kconfig     |    2 +
> >  drivers/virt/coco/sev-guest/sev-guest.c |   87 +++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> > index da2d7ca531f0..bce43d4639ce 100644
> > --- a/drivers/virt/coco/sev-guest/Kconfig
> > +++ b/drivers/virt/coco/sev-guest/Kconfig
> > @@ -2,9 +2,11 @@ config SEV_GUEST
> >         tristate "AMD SEV Guest driver"
> >         default m
> >         depends on AMD_MEM_ENCRYPT
> > +       depends on KEYS
> >         select CRYPTO
> >         select CRYPTO_AEAD2
> >         select CRYPTO_GCM
> > +       select TSM_KEYS
> >         help
> >           SEV-SNP firmware provides the guest a mechanism to communicate with
> >           the PSP without risk from a malicious hypervisor who wishes to read,
> > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > index f48c4764a7a2..2bdca268272d 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/psp-sev.h>
> >  #include <uapi/linux/sev-guest.h>
> >  #include <uapi/linux/psp-sev.h>
> > +#include <keys/tsm.h>
> >
> >  #include <asm/svm.h>
> >  #include <asm/sev.h>
> > @@ -769,6 +770,84 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> >         return key;
> >  }
> >
> > +static int sev_auth_new(struct tsm_key_payload *t, void *provider_data)
> > +{
> > +       struct snp_guest_dev *snp_dev = provider_data;
> > +       const int report_size = SZ_16K;
> > +       const int ext_size =
> > +               PAGE_ALIGN_DOWN(TSM_DATA_MAX - report_size - sizeof(*t));
> > +       int ret;
> > +
> > +       if (t->pubkey_len != 64)
> > +               return -EINVAL;
> 
> Magic number?
> 
> We only support asymmetric keys with public keys exactly equal to 64
> bytes? Is that only p256? SNP uses p384 can we atleast allow that
> curve too? But why not let userspace what key type they want to use?

The kernel has no control here. See Table 20 MSG_REPORT_REQ Message
Structure (https://www.amd.com/system/files/TechDocs/56860.pdf)

...only 64-byte payloads are accepted. I assume one could specify less
than 64-bytes and zero-fill the rest, but that's a contract between the
requester and the attester.

> 
> > +
> > +       if (t->auth_blob_format[0] &&
> > +           strcmp(t->auth_blob_format, "extended") != 0)
> > +               return -EINVAL;
> > +
> > +       if (t->auth_blob_format[0]) {
> > +               u8 *buf __free(kvfree) =
> > +                       kvzalloc(report_size + ext_size, GFP_KERNEL);
> > +
> > +               struct snp_ext_report_req req = {
> > +                       .data = { .vmpl = t->privlevel },
> > +                       .certs_address = (__u64)buf + report_size,
> > +                       .certs_len = ext_size,
> > +               };
> > +               memcpy(&req.data.user_data, t->pubkey, 64);
> 
> Again without any freshness from the remote party, of what use is this
> attestation report?

This interface is just marshaling the same data that could be retrieved
via SNP_GET_REPORT ioctl on the sevguest chardev today. So I would point
you back to vendor documentation for how this report is used. See "VM
Launch and Attestation" here:

https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

I am just here to stanch the proliferation of new chardevs and new
ioctls for this TSM-common operation. This effort was started when TDX
patches showed up to take a 64-byte input payload and wrap it in a
attestation report with its own chardev and ioctls.

> 
> > +
> > +               struct snp_guest_request_ioctl input = {
> > +                       .msg_version = 1,
> > +                       .req_data = (__u64) &req,
> > +                       .resp_data = (__u64) buf,
> > +               };
> > +
> > +               ret = get_ext_report(snp_dev, &input, SNP_KARG);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               no_free_ptr(buf);
> > +               t->auth_blob = buf;
> > +               t->auth_blob_len = report_size + ext_size;
> > +               t->auth_blob_desc = "sev";
> > +       } else {
> > +               u8 *buf __free(kvfree) = kvzalloc(report_size, GFP_KERNEL);
> > +
> > +               struct snp_report_req req = {
> > +                       .vmpl = t->privlevel,
> > +               };
> > +               memcpy(&req.user_data, t->pubkey, 64);
> > +
> > +               struct snp_guest_request_ioctl input = {
> > +                       .msg_version = 1,
> > +                       .req_data = (__u64) &req,
> > +                       .resp_data = (__u64) buf,
> > +               };
> > +
> > +               ret = get_report(snp_dev, &input, SNP_KARG);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               no_free_ptr(buf);
> > +               t->auth_blob = buf;
> > +               t->auth_blob_len = report_size;
> > +               t->auth_blob_desc = "sev";
> > +       }
> 
> Can we reduce the code duplication between the branches here?

I'll take a look.