Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy <aik@amd.com>]

On 17/10/23 16:35, Alexey Kardashevskiy wrote:
> 
> On 17/10/23 15:07, Dan Williams wrote:
>> Alexey Kardashevskiy wrote:
>>> On 13/10/23 13:14, Dan Williams 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 CONFIG_TSM_REPORTS to retrieve the data that
>>>> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
>>>> retrieving the report blob via the TSM interface utility,
>>>> assuming no nonce and VMPL==2:
>>>>
>>>>       report=/sys/kernel/config/tsm/report/report0
>>>>       mkdir $report
>>>>       echo 2 > $report/privlevel
>>>>       dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>
>>> Is not this one a "nonce"?
>>
>> No, a nonce would come from the verifier to be mixed with the 64-bytes
>> that the guest uses to establish a shared secret. In this case this is
>> just showing the mechanics of the ABI with those security best practices
>> set aside.
>>
>>>
>>>>       hexdump -C $report/outblob # SNP report
>>>>       hexdump -C $report/auxblob # cert_table
>>>>       rmdir $report
>>>>
>>>> Given that the platform implementation is free to return empty
>>>> certificate data if none is available it lets configfs-tsm be 
>>>> simplified
>>>> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
>>>> SNP_GET_REPORT alone.
>>>>
>>>> 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 collaboration.
>>>>
>>>> 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>
>>>> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>>> Tested-by: Kuppuswamy Sathyanarayanan 
>>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>> ---
>>>>    drivers/virt/coco/sev-guest/Kconfig     |    1
>>>>    drivers/virt/coco/sev-guest/sev-guest.c |  133 
>>>> +++++++++++++++++++++++++++++++
>>>>    2 files changed, 134 insertions(+)
>>>>
>>>> diff --git a/drivers/virt/coco/sev-guest/Kconfig 
>>>> b/drivers/virt/coco/sev-guest/Kconfig
>>>> index da2d7ca531f0..1cffc72c41cb 100644
>>>> --- a/drivers/virt/coco/sev-guest/Kconfig
>>>> +++ b/drivers/virt/coco/sev-guest/Kconfig
>>>> @@ -5,6 +5,7 @@ config SEV_GUEST
>>>>        select CRYPTO
>>>>        select CRYPTO_AEAD2
>>>>        select CRYPTO_GCM
>>>> +    select TSM_REPORTS
>>>>        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 e5f8f115f4af..f3ca083127af 100644
>>>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>>>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
>>>> @@ -16,10 +16,12 @@
>>>>    #include <linux/miscdevice.h>
>>>>    #include <linux/set_memory.h>
>>>>    #include <linux/fs.h>
>>>> +#include <linux/tsm.h>
>>>>    #include <crypto/aead.h>
>>>>    #include <linux/scatterlist.h>
>>>>    #include <linux/psp-sev.h>
>>>>    #include <linux/sockptr.h>
>>>> +#include <linux/cleanup.h>
>>>>    #include <uapi/linux/sev-guest.h>
>>>>    #include <uapi/linux/psp-sev.h>
>>>> @@ -768,6 +770,129 @@ static u8 *get_vmpck(int id, struct 
>>>> snp_secrets_page_layout *layout, u32 **seqno
>>>>        return key;
>>>>    }
>>>> +struct snp_msg_report_resp_hdr {
>>>> +    u32 status;
>>>> +    u32 report_size;
>>>> +    u8 rsvd[24];
>>>> +};
>>>> +#define SNP_REPORT_INVALID_PARAM 0x16
>>>
>>> There is one already - SEV_RET_INVALID_PARAM, defined in "Secure
>>> Encrypted Virtualization API".
>>
>> Ok, indeed I took this from Jeremi without checking for other
>> definitions.
>>
>>>
>>>> +#define SNP_REPORT_INVALID_KEY_SEL 0x27
>>>
>>> This one needs to be defined in include/uapi/linux/psp-sev.h's 
>>> sev_ret_code.
>>
>> Ah, ok, will move.
>>
>>>
>>>> +
>>>> +struct snp_msg_cert_entry {
>>>> +    unsigned char guid[16];
>>>> +    u32 offset;
>>>> +    u32 length;
>>>> +};
>>>> +
>>>> +static int sev_report_new(struct tsm_report *report, void *data)
>>>> +{
>>>> +    static const struct snp_msg_cert_entry zero_ent = { 0 };
>>>> +    struct snp_msg_cert_entry *cert_table;
>>>> +    struct tsm_desc *desc = &report->desc;
>>>> +    struct snp_guest_dev *snp_dev = data;
>>>> +    struct snp_msg_report_resp_hdr hdr;
>>>> +    const int report_size = SZ_4K;
>>>> +    const int ext_size = SEV_FW_BLOB_MAX_SIZE;
>>>
>>> These two are size_t. Or u32. "int" is just weird :)
>>
>> Nothing is bigger than a few kb here, but ok.
> 
> If it all was just "unsinged", I would not bother commenting :)
> 
>>
>>>
>>>> +    int ret, size = report_size + ext_size;
>>>> +    u32 certs_size, i;
>>>
>>> @certs_size is size_t (as it is copied to ->auxblob_len in the end), and
>>> @size is size_t as well.
>>>
>>> @i is just "unsigned", can be declared right in the "for" below?
>>
>> ok.
>>
>>>
>>>> +
>>>> +    if (desc->inblob_len != 64)
>>>
>>> 64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
>>> May be even BUILD_BUG_ON(TSM_INBLOB_MAX != 
>>> sizeof(ext_req.data.user_data)) ?
>>>
>>>> +        return -EINVAL;
>>>> +
>>>> +    void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
>>>
>>> I did not realize declaring variables in a middle of a scope is allowed
>>> now :)
>>
>> Yes, it's been allowed in loop declaration for a few kernels now and the
>> new __free() helper in cleanup.h will make this model more prominent. My
>> sense is that it is still not open season on mid-function declarations,
>> but __attribute__((__cleanup__())) usage needs it.
>>
>>> Since you are doing this, move zero_ent below. Or, better, use
>>> guid_is_null().
>>
>> guid_is_null() is not techinically enough given the specification
>> mandates that the entire entry is zero.
> 
> Well technically it says that guid, offset and length must be zeroes. 
> So, (guid_is_null() && !offset && !length) and no yet another static 
> declaration of a bunch of zeroes far away. I even wonder if there is a 
> helper to check if some memory is all zeroes :) Up to you.
> 
> 
>>>> +    if (!buf)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    guard(mutex)(&snp_cmd_mutex);
>>>> +
>>>> +    /* Check if the VMPCK is not empty */
>>>> +    if (is_vmpck_empty(snp_dev)) {
>>>> +        dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
>>>> +        return -ENOTTY;
>>>> +    }
>>>> +
>>>> +    cert_table = buf + report_size;
>>>> +    struct snp_ext_report_req ext_req = {
>>>> +        .data = { .vmpl = desc->privlevel },
>>>> +        .certs_address = (__u64)cert_table,
>>>> +        .certs_len = ext_size,
>>>> +    };
>>>> +    memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
>>>> +
>>>> +    struct snp_guest_request_ioctl input = {
>>>> +        .msg_version = 1,
>>>> +        .req_data = (__u64)&ext_req,
>>>> +        .resp_data = (__u64)buf,
>>>> +        .exitinfo2 = 0xff,
>>>
>>> Not sure we need this line with 0xff.
>>>
>>> The GHCB spec says the hypervisor sets it, not the guest. And I could
>>> not figure out why exactly snp_guest_ioctl() does "input.exitinfo2 =
>>> 0xff", my best guest it is to catch GHCB not being called before copying
>>> memory to user.
>>
>> It does mostly seem that way, but given this is plumbed deep into
>> handle_guest_request() I figure might as well keep common semantics.
>>
>>>> +    };
>>>> +    struct snp_req_resp io = {
>>>> +        .req_data = KERNEL_SOCKPTR(&ext_req),
>>>> +        .resp_data = KERNEL_SOCKPTR(buf),
>>>> +    };
>>>> +
>>>> +    ret = get_ext_report(snp_dev, &input, &io);
>>>> +
>>>
>>> Unnecessary empty line.
>>
>> ok.
>>
>>>
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    memcpy(&hdr, buf, sizeof(hdr));
>>>> +    if (hdr.status == SNP_REPORT_INVALID_PARAM)
>>>> +        return -EINVAL;
>>>> +    if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
>>>> +        return -EINVAL;
>>>> +    if (hdr.status)
>>>> +        return -ENXIO;
>>>> +    if ((hdr.report_size + sizeof(hdr)) > report_size)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
>>>> +    if (!rbuf)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
>>>> +    report->outblob = no_free_ptr(rbuf);
>>>> +    report->outblob_len = hdr.report_size;
>>>> +
>>>> +    certs_size = 0;
>>>> +    for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); 
>>>> i++) {
>>>> +        if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
>>>> +            break;
>>>> +        certs_size = max(certs_size, cert_table[i].offset + 
>>>> cert_table[i].length);
>>>> +    }
>>>> +
>>>> +    /* No certs to report */
>>>> +    if (!certs_size)
>>>
>>> Nit: WARN_ON_ONCE(i) here?
>>
>> Seems harsh for what could only be a firmware bug, panic_on_warn users
>> would not appreciate crashing the kernel over something recoverable like
>> this.
> 
> This would a HV bug as certificates come from the KVM. And it is 
> (slightly) more likely that the HV is trying to trigger buffer overrun 
> in the guest.
> 
>>>
>>>> +        return 0;
>>>> +
>>>> +    /*
>>>> +     * cert_table reports more data than fits in ext_size the
>>>> +     * userspace cert_table walker can decide what happens next,
>>>> +     * truncate the output
>>>> +     */
>>>> +    if (certs_size > ext_size)
>>>> +        certs_size = ext_size;
>>>
>>> This sounds more like the HV provided a broken table with offset(s)
>>> ouside of the certs buffer. The HV is expected instead return
>>> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
>>> guest to retry.
>>
>> The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
>> prepared to retry. Retry support would be a follow-on new capability.
> 
> My point is that you should not get into the situation when this 
> calculated certs_size is greater than ext_size. If this is the case 
> because someone sent too many certificaties via /dev/sev or kvmfd on the 
> host, the GHCB call won't return any certs and will ask for a retry 
> instead.
> 
> 
>>>> +
>>>> +    void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>>> +    if (!cbuf)
>>>> +        return -ENOMEM;
>>>
>>> In a such (unlikely) event the function returns an error but does not
>>> free report->outblob which is going to leak if consequent call succeded.
>>> This new no_free_ptr business is confusing at times :(
>>
>> If this fails it results in the attribute read failing and
>> read_generation does not advance. The next read attempt will free the
>> partially completed report and retry,
> 
> Ah ok. In general, it just feels like every use of no_free_ptr() defeats 
> the whole purpose of __free(xxx).
> 
>> or the driver gets unloaded and
>> the partially completed report is freed at that time.
>>
>>>
>>>
>>>> +
>>>> +    memcpy(cbuf, cert_table, certs_size);
>>>> +    report->auxblob = no_free_ptr(cbuf);
>>>> +    report->auxblob_len = certs_size;
>>>
>>>
>>> Aaaand, it works, so:
>>>
>>> Tested-by: Alexey Kardashevskiy <aik@amd.com>
>>
>> Thanks!
>>
>> Did you happen to test @auxblob population? I was not able to test that
>> path on the hardware I tried.
> 
> Yup. I have a disgusting python script which feeds some dummy certs to 
> /dev/sev on the host and checked they travel all the way to this 
> configfs nodes.
> 
> 
> #! /usr/bin/env python3
> 
> #define _IOC_SIZEBITS   13
> #define _IOC_DIRBITS    3
> #define _IOC_NONE       1U
> #define _IOC_READ       2U
> #define _IOC_WRITE      4U
> #define _IOC(dir,type,nr,size) \
> #        (((dir)  << _IOC_DIRSHIFT=30) | \
> #         ((type) << _IOC_TYPESHIFT=8) | \
> #         ((nr)   << _IOC_NRSHIFT=0) | \
> #         ((size) << _IOC_SIZESHIFT=16))
> #define _IOWR(type,nr,size) 
> _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
> 
> import fcntl, array, sys, binascii, pprint, re, uuid, subprocess, os, 
> struct, uuid
> 
> # This is:  cat ~/s/sev/sev-guest | ssh $1 python3 -
> if len(sys.argv) > 1 and '-' not in sys.argv:
>      remote_host = sys.argv[1]
>      print("*** Running remotely on {}".format(remote_host))
>      f = open(os.path.realpath(__file__), 'r')
>      ssh = subprocess.Popen(["ssh", remote_host, "python3", "-"], stdin 
> = f)
>      ssh.communicate()
>      sys.exit(0)
> 
> 
> _IOC_READ = 2
> _IOC_WRITE = 4
> 
> def _IOC(d, t, n, s): return (d << (16 + 13)) | (t << 8) | n | (s << 16)
> def _IOWR(t, n, s): return _IOC(_IOC_READ | _IOC_WRITE, t, n, s)
> 
> SEV_IOC_TYPE = ord('S')
> sev_issue_cmd_size = 16
> SEV_ISSUE_CMD = _IOWR(SEV_IOC_TYPE, 0x0, sev_issue_cmd_size)
> 
> SNP_SET_EXT_CONFIG = 10
> 
> def sev_issue_cmd(fd, cmd, data):
>      data_ptr = data.buffer_info()[0]
>      arg = struct.pack("=LQL", cmd, data_ptr, 0)
>      try:
>          arg = fcntl.ioctl(fd, SEV_ISSUE_CMD, arg, True)
>      except OSError as x:
>          return x.errno, 0
>      _, _, err = struct.unpack("=LQL", arg)
>      return 0, err
> 
> # struct cert_table {
> #    struct {
> #        unsigned char guid[16];
> #        uint32 offset;
> #        uint32 length;
> #    } cert_table_entry[];
> # };
> def cert_table_entry(cert_table):
>      ret = array.array('B')
>      hdr = array.array('B')
>      offset = (16 + 4 + 4) * (len(cert_table) + 1)
>      i = 0
>      for i, c in enumerate(cert_table):
>          cert = array.array('B', [ord('0') + i] * 32)
>          ret += cert
>          hdr += array.array('B', c.bytes)
>          hdr += array.array('B', struct.pack("=LL", offset, len(cert)))
>          offset += len(cert)
> 
>      return hdr + array.array('B', [0] * 24) + ret
> 
> myuuids = [uuid.UUID("f50ec0b7-f960-400d-91f0-c42a6d44e3d0"),
>          uuid.UUID("9f3d7b34-6c0d-11ee-bcd4-f8e4e3857730"),
>          uuid.UUID("aabbccdd-eeff-0011-2233-4567890abcde")]
> cert_table = cert_table_entry(myuuids)
> cert_table += array.array('B', [0] * (4096 - len(cert_table)))
> 
> # sev_user_data_ext_snp_config
> ext_config = array.array('B', struct.pack("=QQL", 0, 
> cert_table.buffer_info()[0], cert_table.buffer_info()[1]))
> 
> sev = open("/dev/sev", "w")
> ret, err = sev_issue_cmd(sev, SNP_SET_EXT_CONFIG, ext_config)
> print("ret = {} err = {}".format(ret, err))
> 

Ah the script's result as it appears in the guest, just in case:


aik@aik-Standard-PC-i440FX-PIIX-1996:~$ hexdump -vC 
/sys/kernel/config/tsm/report/report0/auxblob
00000000  f5 0e c0 b7 f9 60 40 0d  91 f0 c4 2a 6d 44 e3 d0 
|.....`@....*mD..|
00000010  60 00 00 00 20 00 00 00  9f 3d 7b 34 6c 0d 11 ee  |`... 
....={4l...|
00000020  bc d4 f8 e4 e3 85 77 30  80 00 00 00 20 00 00 00 
|......w0.... ...|
00000030  aa bb cc dd ee ff 00 11  22 33 45 67 89 0a bc de 
|........"3Eg....|
00000040  a0 00 00 00 20 00 00 00  00 00 00 00 00 00 00 00  |.... 
...........|
00000050  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
|................|
00000060  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 
|0000000000000000|
00000070  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 
|0000000000000000|
00000080  31 31 31 31 31 31 31 31  31 31 31 31 31 31 31 31 
|1111111111111111|
00000090  31 31 31 31 31 31 31 31  31 31 31 31 31 31 31 31 
|1111111111111111|
000000a0  32 32 32 32 32 32 32 32  32 32 32 32 32 32 32 32 
|2222222222222222|
000000b0  32 32 32 32 32 32 32 32  32 32 32 32 32 32 32 32 
|2222222222222222|



-- 
Alexey