linux-kernel - Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 14 Apr 2023 10:40:54 +0530
From:   "Nikunj A. Dadhania" <nikunj@....com>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, bp@...en8.de,
        thomas.lendacky@....com, dionnaglaze@...gle.com, seanjc@...gle.com,
        pbonzini@...hat.com, michael.roth@....com, ketanch@...k.ac.in
Subject: Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

On 4/10/2023 10:44 PM, Peter Gonda wrote:
>> +
>>  /* #VC handler runtime per-CPU data */
>>  struct sev_es_runtime_data {
>>         struct ghcb ghcb_page;
>> @@ -1107,7 +1111,7 @@ static void *alloc_shared_pages(size_t sz)
>>         return page_address(page);
>>  }
>>
>> -static int snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
>> +static int __init snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
>>  {
>>         u64 gpa;
>>         int ret;
>> @@ -1406,6 +1410,80 @@ bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>>  }
>>  EXPORT_SYMBOL_GPL(snp_assign_vmpck);
>>
>> +static int __init snp_get_tsc_info(void)
>> +{
>> +       u8 buf[SNP_TSC_INFO_REQ_SZ + AUTHTAG_LEN];
>> +       struct snp_tsc_info_resp tsc_resp = {0};
>> +       struct snp_tsc_info_req tsc_req;
>> +       struct snp_guest_req req;
>> +       struct snp_guest_dev dev;
>> +       int rc, resp_len;
>> +
>> +       /*
>> +        * The intermediate response buffer is used while decrypting the
>> +        * response payload. Make sure that it has enough space to cover the
>> +        * authtag.
>> +        */
>> +       resp_len = sizeof(tsc_resp) + AUTHTAG_LEN;
>> +       if (sizeof(buf) < resp_len)
>> +               return -EINVAL;
>> +
>> +       /* Zero the tsc_info_req */
>> +       memzero_explicit(&tsc_req, sizeof(tsc_req));
>> +       memzero_explicit(&req, sizeof(req));
> 
> Whats the guidance on when we should use memzero_explicit() vs just
> something like: `snp_tsc_info_resp tsc_resp = {0};`?

Going over the history of memzero_explicit, it seems it was introduce to 
explicitly zero sensitive information before the variable goes out of scope. 
GCC was optimizing out the memset in these cases:

d4c5efdb9777 ("random: add and use memzero_explicit() for clearing data")

https://bugzilla.kernel.org/show_bug.cgi?id=82041

With the above detail, IMHO, we do not need the memzero_explicit() for both case.

> 
>> +
>> +       dev.pdata = platform_data;
>> +       if (!snp_assign_vmpck(&dev, 0))
>> +               return -EINVAL;
>> +
>> +       req.msg_version = MSG_HDR_VER;
>> +       req.msg_type = SNP_MSG_TSC_INFO_REQ;
>> +       req.req_buf = &tsc_req;
>> +       req.req_sz = sizeof(tsc_req);
>> +       req.resp_buf = buf;
>> +       req.resp_sz = resp_len;
>> +       req.fw_err = NULL;
> 
> Why do we not want the FW error code?

I will add the FW error code.

Regards
Nikunj