Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Cc:     brijesh.singh@amd.com, 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 <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ard Biesheuvel <ardb@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andi Kleen <ak@linux.intel.com>, tony.luck@intel.com,
        marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com
Subject: Re: [PATCH Part1 v5 37/38] virt: sevguest: Add support to derive key
To:     Borislav Petkov <bp@alien8.de>
References: <20210820151933.22401-1-brijesh.singh@amd.com>
 <20210820151933.22401-38-brijesh.singh@amd.com> <YTjB/KTBsqExqylc@zn.tnic>
From:   Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <f2d29f4d-f020-0036-c2c8-2b4e993d3605@amd.com>
Date:   Wed, 8 Sep 2021 16:44:37 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
In-Reply-To: <YTjB/KTBsqExqylc@zn.tnic>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MGNtUTdSUkNzVFEwYzZzTTgwOW9iSFJZb3J6aTJpRDZoT1hzRWw1YnRUblov?=
 =?utf-8?B?WWwwQmwxWnpueUQyVlhNaC9vUjJMc3IzbTdSaEVQdTdmYzRIV1NTWk5hWnVO?=
 =?utf-8?B?TVI0V1h4ejg1eGFwblU3d0hJeFZVZ0RRQUdHNC9DaFVnNlRDSEV6b2VZbi9B?=
 =?utf-8?B?b1VzQ0ludTIzdXo2WkZpMmNxVVE0NDByRXUyRlVmSUVVeHZoMEgzVkozVTJv?=
 =?utf-8?B?Tld3a3c2YmQ5eDJwUlM2eHhYNGo5VTBHSGFQUnlEdEwycW5BMUlKUzBNYWVU?=
 =?utf-8?B?ODA1aUVHU0xYNWV0MHEvYnRxQ0tkY3NYN1pqRWEybnRYc1ZSTmJWdXRkbTQw?=
 =?utf-8?B?VUN3VzZ3WmFKclZ6NHlRa21UbHpsY3NqWFJSTGtVZko2VDB0VitjMGk3eHlU?=
 =?utf-8?B?OFg0MTZGSFBlQVQ5WVYrUVNGSHVPU1J5Vk00Mm0xd2dONVBjT0pudDc2OU1y?=
 =?utf-8?B?SW1PM09IZDVHeCttYTNzeXhkVHl3NTZ1WFRxbXlCY0NZQ0NTelJVWkNXZmpM?=
 =?utf-8?B?a0VEYnVERFcxamo0ZzcyZjJpVTNPMGxld3NTVHpMU0doMCtCMFFQdGlhZFdy?=
 =?utf-8?B?OUJXcEliR1ZLSS9NdUpjcTljREJxM3F3czBSc1l1UWF6dE15WGpwZjZyUmh2?=
 =?utf-8?B?NnNoR0orM3RRaGk2bjNTaS8rcDl2VlZLblNQTHdXRnlhNDc5c3FPUEQ2RkF3?=
 =?utf-8?B?bUxBaHFKbS83akd2UHFCNWxiQmNUbFZWR1V0ODNBTytmYmdSMEU3YUNLdmg1?=
 =?utf-8?B?L0FhWG1mTTJ4YXY3bVZjeUFua0VwWVBXeks2dkV3UG1JeFJybmYzck0rRHlr?=
 =?utf-8?B?UjgzM1I4Um5FN0tyTkxsMjhtSHBDKytxQmQ0bm5ka24zOXN2eS8yZHVzL3Jw?=
 =?utf-8?B?bjdnei93Qml1SHg5T0RLSGlQeUxNR2pVcEhKQXFuUnFvN1NjNWhYVzA2NGcz?=
 =?utf-8?B?NS85ajZ6V0w1TGdYTzd6QjZhYm9rRmJ5dW1wOHQ5Q3BjSlBXWGw2ekJ2b3pQ?=
 =?utf-8?B?ZUlGOEJGSzlqOHBxalh1RGZiRFBIZ0dORHBMM0pIejUyeDN5K09GczBVK0tB?=
 =?utf-8?B?c3VDay9DZ0NXLzFIOEdneWt2S3NVNnY2ekNpRVVEWG9rTnZGYUhtVDFhYmJx?=
 =?utf-8?B?eWk0YmRmWEFMZ0hCanphWjg5TnNhRXVBcjNoeFVoaEM1SHVzdE14eGtvVVZS?=
 =?utf-8?B?WllZK1RvOWNrSUJYZ0MzUTVwbnFPZFc0MVRodnVINTVxcHN1QkowTWU0ZkF2?=
 =?utf-8?B?eUljTEtCL0l0a3Fzc0ZaNW8xdEhFZEJ0MGh3bFp4TmRnMko2QzIxc0V2VWt1?=
 =?utf-8?B?QTdOVzNpRE93T0ZSaDNiUkNyaGxkRzRCR29zTnhWRVZja3UyaFp6S3V1UEds?=
 =?utf-8?B?VWtoYXNXVHlkZDlVb1NnY1cxMndRelNRYlZlbnBSa2RwWE00VjcxVU4yeGta?=
 =?utf-8?B?Tk9HMm0ybEhUTE5kL2V2ZytQV3ZXWVBjK2VTRXV3b0xGMUNwZm44UXc4eU1N?=
 =?utf-8?B?Sndpa3hzTEc5a0gzSTlRUllvWWwya1BUY2R5MUJ6NGZVaFhYTU42dTdWRUFy?=
 =?utf-8?B?NEVSMkJUUmRsR3EzaFJpQW9JaFpFRDRpVm9zSUpSV3B6UlNjT2piTGh6R1ZN?=
 =?utf-8?B?TzF0bDJXOXNnSWwxUUtKY1VKMnJCNC9kZmNWcVovNTFGNjBzVHhZS1hBc0pJ?=
 =?utf-8?B?dC9jUGxwNlNHOXFKNzl2b0ZEakNsclMvUWZsc0ptMFhUeHNpVkg1YzJWUWRp?=
 =?utf-8?Q?nVotyPr5ELl1FA6g1QWjvE1ICzD+ukHX3l4B7g6?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cce7ef9-2560-414a-f2c8-08d97311dc22
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Sep 2021 21:44:39.8799
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: f7UQQANIi3yxVWRs77KkhAwKsOGGC0uiUGueNn6HyudlGt1BehxMcR+0c5+HXN5aaFITblbp77R5ybCz15tpCg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4382
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/8/21 9:00 AM, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote:
>> +2.2 SNP_GET_DERIVED_KEY
>> +-----------------------
>> +:Technology: sev-snp
>> +:Type: guest ioctl
>> +:Parameters (in): struct snp_derived_key_req
>> +:Returns (out): struct snp_derived_key_req on success, -negative on error
>> +
>> +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key.
>> +The derived key can be used by the guest for any purpose, such as sealing keys
>> +or communicating with external entities.
>> +
>> +The ioctl uses the SNP_GUEST_REQUEST (MSG_KEY_REQ) command provided by the
>> +SEV-SNP firmware to derive the key. See SEV-SNP specification for further details
>> +on the various fileds passed in the key derivation request.
>> +
>> +On success, the snp_derived_key_resp.data will contains the derived key
> 
> "will contain"

Noted.

>> +
>> +	/* Copy the request payload from the userspace */
> 
> "from userspace"

Noted.


>> +
>>   static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>>   {
>>   	struct snp_guest_dev *snp_dev = to_snp_dev(file);
>> @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>>   		ret = get_report(snp_dev, &input);
>>   		break;
>>   	}
>> +	case SNP_GET_DERIVED_KEY: {
>> +		ret = get_derived_key(snp_dev, &input);
>> +		break;
>> +	}
> 
> {} brackets are not needed.
> 
> What, however, is bothering me more in this function is that you call
> the respective ioctl function which might fail, you do not look at the
> return value and copy_to_user() unconditionally.
> 
> Looking at get_derived_key(), for example, if it returns after:
> 
>          if (!arg->req_data || !arg->resp_data)
>                  return -EINVAL;
> 
> you will be copying the same thing back to the user, you copied in
> earlier. That doesn't make any sense to me.

I will look into improving it to copy back to userspace only if there is 
firmware error.

thanks