From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753618AbdJMCYN (ORCPT ); Thu, 12 Oct 2017 22:24:13 -0400 Received: from mail-bn3nam01on0041.outbound.protection.outlook.com ([104.47.33.41]:30849 "EHLO NAM01-BN3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751657AbdJMCYK (ORCPT ); Thu, 12 Oct 2017 22:24:10 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Cc: brijesh.singh@amd.com, Paolo Bonzini , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Herbert Xu , Gary Hook , Tom Lendacky , linux-crypto@vger.kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command To: Borislav Petkov References: <20171004131412.13038-13-brijesh.singh@amd.com> <20171007010607.78088-1-brijesh.singh@amd.com> <20171007010607.78088-7-brijesh.singh@amd.com> <20171012195331.bdzwqzyrjc6fi5lj@pd.tnic> From: Brijesh Singh Message-ID: Date: Thu, 12 Oct 2017 21:24:01 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171012195331.bdzwqzyrjc6fi5lj@pd.tnic> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: DM3PR12CA0083.namprd12.prod.outlook.com (2603:10b6:0:57::27) To DM2PR12MB0155.namprd12.prod.outlook.com (2a01:111:e400:50ce::18) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7b45ce58-3946-4e6c-5ae4-08d511e17a78 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(48565401081)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:DM2PR12MB0155; X-Microsoft-Exchange-Diagnostics: 1;DM2PR12MB0155;3:zBZT4S41aVGGCf4VCf6nfIe4vtIQHme8Slp5+z9KPEo6WSgZMxtGYIc4c5KmQoXipT8+qVr76LXfxUORcLU+1uAJBiyRtIQg3WH9oDEe9TF/Sy3jc9vA6xxEGhiwXw3Qy4J1u738b2KCXCDxr7/Tej8t7kj14T0fVO2Vk3vcivKtZt/id8dYxu/GtN/xsKhgLrJlaDaK1vd//vUN6ORGh1QPSYFeaJO7rfZ8vuvPKE4avXNYrstdMyqQGvOBLv5Q;25:1vQmAqrMMK9yQP1neWS+7aaTCoyv5rGpCMSrznt8M+DqyHDmK2826eMDjmUqLochGosRI4DuHbeIhXPUlKrImZoWfoDLrHN6IJNr0Vjgf1JjIWfZSa+WJ9NY2FvTuMgulETz/w0gGGpXEW42Fu7wkCpgGyRE+jkxDL/zgXmvd2DpIwb7qxGuI9tPs3BIDnpmoPmz4zJB0fVCDLIj3hTgTKWqcf8hKRP/JyrFdftMmTtH5Nmfb1ZW0uPhNKoCNMwrnzAUjdVdaRCjzHpiPrfyjZXqLF810eF/r82F/sTIaRYBc/AfzeBGdTBAOZ57GCzTu0tq/t0RWvtsTD12OpGj9w==;31:flwVP0MVtV4s16B2Yc7WuoBE0Lk82cAE2cE3p1oLTtFRzodQ0eebJUA07cBlAA+bFx8S41tK6hse1WpXIIkf/WO4wXbrDhrZEvqEIDBLjKnVACY9VzDFhH66YGqBZscKiDA0CcTRQM1abX422WPVYJWkelCAy9NhRY9mDMpLtQHdGrRvjgwgVHL3Z0zNorn3TgC+Ct+y2s2LsdmRI+7bcz511/fZpfylxlcqd7ykPeY= X-MS-TrafficTypeDiagnostic: DM2PR12MB0155: X-Microsoft-Exchange-Diagnostics: 1;DM2PR12MB0155;20:NGuoqcQCUTKKfLxd2xHYDXnBgRAPnD17HUKo4ix6Bhed+ic+/sIIMsPJKBRuhD8ReoC8Jtx04LUmMJponKecND/5j5mlBug6Us9+yJVB9QMgId6UDJmtu3t2E08wG81j/P2IKOGLHcy52lcaDKxjTSp3kci0oha4G+fl215ZgWJQyeP2Kjy0iW4BZnwcr0C/1ovlK74KzrN5TKl1KzstosaTwdpwJouawDCKDkWbBaZmZJA8MDG2/yhrjVQgxuhIGPcspdJgl1MVO7+yFP4qZ8tMxo2RSxS9vbql/6V9Mu6bQUuwU8dhdiz6KNWjUpPeg2qjRs/86k/zErK4kLLefIxqiBKY/HTE3z/o9xSB4mZOK5QTTWDBOH/dcteSCcCizGvl65IaFfvgATD9PjXoQrqLRWEuLmYN6vd3z8Nus1rmL1EEzfHKulZDzQs3Mv+wtUFQl6a2Gi+ApLiE4YoE2BQf3nAINzZvbAzvsJR85LAHBSC3/qs1HX0wghH1QTSA;4:KMl2hakAFB0ipUUAD9+4vDOUiY91VP8LOkMFRzt1tAEKNXChS+UUx/Hhzd8XgE256ThuHjuj6/feWD7l0szAfWHTfdmIDGHeL+AYZxtVlHHGiacfa2TO3b9GMLZORp4UppER650DNoVDJ9JwRZZH2vJ9C6CceMioS1ql0/HUD06zTCuI/FfS/JCiD2CwxaqxdgQoUVvZv3oAKmlRKILkvstOiFz0DD5x4m8I1kDgpZjf7H477i0CZe0/26OQBhlS X-Exchange-Antispam-Report-Test: UriScan:; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(3002001)(100000703101)(100105400095)(93006095)(93001095)(10201501046)(6055026)(6041248)(20161123558100)(20161123555025)(20161123560025)(20161123562025)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:DM2PR12MB0155;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:DM2PR12MB0155; X-Forefront-PRVS: 04599F3534 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(979002)(6009001)(39860400002)(376002)(346002)(5423002)(199003)(189002)(24454002)(377454003)(86362001)(6916009)(2950100002)(3846002)(4326008)(189998001)(305945005)(47776003)(97736004)(23676002)(65806001)(31686004)(66066001)(6666003)(65956001)(6116002)(16526018)(7736002)(25786009)(81156014)(81166006)(8936002)(83506001)(6486002)(230700001)(50466002)(53546010)(54906003)(36756003)(229853002)(8676002)(33646002)(316002)(65826007)(6246003)(105586002)(53936002)(2906002)(5660300001)(58126008)(68736007)(31696002)(53416004)(64126003)(54356999)(93886005)(478600001)(50986999)(106356001)(76176999)(101416001)(969003)(989001)(999001)(1009001)(1019001);DIR:OUT;SFP:1101;SCL:1;SRVR:DM2PR12MB0155;H:wsp094129wss.amd.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtETTJQUjEyTUIwMTU1OzIzOjNrNXpkbWdGMGxUUzh4azJKbFEyRm1lUU5C?= =?utf-8?B?OWVLL1dwcjRWVkVHRmFNaUJQSURJOUF3VlRLd0RRUnljZ1dLM21TOTJNajB3?= =?utf-8?B?MEZNOVc2N0lZMmIvT1NETHNONlU3Qm4ySXpaVmRCRkpuVEFIempXZmdoTE9E?= =?utf-8?B?blh0aVpseThjaUFoV092YTNIMitpTnkrVDRpaUhaMmNZcTl0MmlTZGtiNjJ4?= =?utf-8?B?ZmtBRGV4eEpoMnk5R3BxTDFSV1BMaE1LY2tSV0ZmQVFFSFlvV052OXBRcktV?= =?utf-8?B?V1UrVXRHbnlMaDJ0VHZkRDZxZHF0RXA2TGZHczlCSDRGT1U2UlhUNlVXUTY4?= =?utf-8?B?KytxNFQ1Z1NzN0V6S3JrdkxOUkNZQ1hYL1p1bThjVHJ0K0k1UUx2bm9MK1JZ?= =?utf-8?B?MjB1clNTUE8zZGFCMjZyalFqQ0poSnpYTGV5ZyswS1lmenYvbjRGVURqNm0v?= =?utf-8?B?NXgwODRXODdLQ1FWQWVGRW5CazRWbmlacDd0b2w3QzdkNlJ1a004Y3VreUJE?= =?utf-8?B?aCtqREl2dG9rMTRDVDlQeHBVZGZRUG5HRmJibyt4YXp6RTBIWlBFdThpamFZ?= =?utf-8?B?MjZpZ2lRaEEyNERtS1Z1WExwSFV4UUlKc1NuRmdGNndHTGZDY2VFeENGSzkz?= =?utf-8?B?dGNQSjFFTTltZWNrY2I2NVRrTzJkWCtJcWxMZ05OcXVrVm9IYUI1RWJwaUdN?= =?utf-8?B?N3FESWJSbUQvWFplWk83MlFsSU96bU5qUTFJZC9HUkEwOFpsK2dRM3Eyaytx?= =?utf-8?B?RktMand2OFJ4VkR1bUZacU13Y3Y5QkhuS0FubzJncmpKYUFlTWFObkIrTXJQ?= =?utf-8?B?aDh2Vm1Ib1N5VTVFaDlwWmlIazd6eVpVZTF0T0pVSWZMVDlpU3JvNEE3ejk1?= =?utf-8?B?emlRZVJXUlg2OXloY3U3d0RNOTZScmx3MkdYc3FpWXpnM1cyWWhOM2JJTXNo?= =?utf-8?B?OHFhT3JwWURGa0hIdWVXbTJOSWxCWWJ2QVRaeTk4NDNpQ0VyZm9STXZLVVZy?= =?utf-8?B?S0hhbDhkWHBpM1NxRkd3K21NcnJwSmVmakZBSldaOWNBbnREemtNd0MyMkUz?= =?utf-8?B?YjVhVk9iQ3NHd1RaeGNkZW9sTjJycjJNY2NLTEdRSGtBSFVyZWZ1d2hucDBy?= =?utf-8?B?R29kYmNHOW1yaFovTThIcEx4YjltdEljRkEwOGZkbUg5KzIyU1Nyek5HSDNB?= =?utf-8?B?Nk83VjRvejM5amc3bVdlMmFvWXo3c0JoRWRrRFdHZGErV2l2SkErWGpEZzN1?= =?utf-8?B?Ykw3QjBXNndZSGduYUdCQkFYRkx3ek1RRDZ0VHR6aGlvZmZMK1gycVAzdzFS?= =?utf-8?B?RGtRRGVKVVI4VlhtMW1VLzFVRzNVWEhwSnhwblcveWtzb2U2bms5NUNyc3Zi?= =?utf-8?B?WTdFYmFVSEFXVEZ3UUcvK3BOdE9yMEZ3aWh2b2tzVFh2SEJCallxR1l3aVJH?= =?utf-8?B?SjgwWjZzSlIyUkl0YWN0QjZoOERQUTNMRE43OS9Qb0RqQjVxK25KUTRUc3BT?= =?utf-8?B?Z09CMmV0L3J5eEVzRnM1LzR5VVppd0NPZEVnNEw4U3JiWlYvcm1hbHkyQ3V0?= =?utf-8?B?b2Mzb0lFRVUxMzBGSUN5UWNRS0hsRmdIUU1sTVRzRHhGNzQ2TlptMVpaYVc2?= =?utf-8?B?NFpQVVdUeUpGOFM2OGE4UGNGQjQ0RCtLTnFTYW1lVlVsNjVLTFF2ckZkU21V?= =?utf-8?B?RWhmRmRjZnlqalFoZC83WWx3Y3l2aTQxWnlaZDMvV1hnZ0RmWkZ0TkQzOVhz?= =?utf-8?B?b08vZXJ2cXBvTlM2RVNOakEwSXNGWDVHSWt4VmVYNXVBTDF2RzBTVHQ5STJS?= =?utf-8?B?SnpaSmRlQVRRazZVT3V4QkllMUdYa1VpSWhQUXp2bWV4UDNJT1FNNWJKNTZF?= =?utf-8?B?Ynpzd1Izc1ZWQlN5TXZRVzBqRVhVOVJnVjV2WlRQRjgwUDI4bHVTc1g4eVVw?= =?utf-8?B?NjRvRzZ5ZStXUCtaL09wTSt2aktFaGxuV2FNOWhvWDc4WmIyTmpxd0FBZHJ0?= =?utf-8?B?TGNiVjg5aXdHcGFJTlIvczlJYXpDRzVubWtyUT09?= X-Microsoft-Exchange-Diagnostics: 1;DM2PR12MB0155;6:U7IaAzpl4JB/wjcU/1EcwFsHRolp3c5jAopXKDEwOa1xaxw72uZhCu6CuhNxRmhX/gaiM0NW/B+WZKW6sCUOfZ3AZQs0OCIzqAlkvCcWEXGDsprvA9c3s8MRns08ipygOEW+ONTUCzaErwwRIJXoT63CrT8Yefg0mbQfO9S23qfbLCBqqApmdshVVKXXYHEoub0NeeE4b1JocBKDbCJ/A44MLHKej47UfaSXLH1zUf6r64WbWHurXKKya1TrEBD2gZSULn1q6JFU33LjNlCqb4qkrne4HPCwCrG7gL3J3ZKBbJmoNaR5BG51/Q00aWnx+OgMqE37gxGa2z/aHcItMg==;5:0H4MprAdB+LY0rOqLKqeMG4duEg77nHQaupF8XjbHmUh5xqzDy5u82ufnhA5MjYGm0JK499g/szrJyC9B+PUqkK0SU+Yu/M0h2v3zKIWHwcvjIztek9s2/6pZE/ws20IDFo6vs2cgc3ziSUi3E6GfA==;24:GMx1mQpzhocOuMcRMnhXYFh8ON59Uwf/FPPcWJhW0KyQoLxLw3BSuatUWAPzWRCiRQwMv2+XriAgEhJMiYwRGFPptcg3XxqhvY4bfqfIAXY=;7:lG/BznUmqsyCGB/7HPzQuggyr0Wx4Qgd+ANvamJDw6kjBK7Wne9p0zM2ZrIZlb2NjVOO3o2quoXS27W+w9mb8+X0tdquYvi9vFh87e2BvycVL9lqHiLHROzOAibMyVtJFB6LbTvGKsPo4jcJXlSvcF/BEkibDDjfwkgHXZYB2C0zIggJMBhDeX4sN+EXCzpf0sPqJvrpW5Tkt3sXrhe1fz6Qv4gW6s4I9na8C/yXxhg= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;DM2PR12MB0155;20:LNs3jvab0uS9NeDVZmAbQ3Jtpok6jTDmsjuYcBEaMbfv/HDijFa2jP+TKP3/ftvQ8MFkEuOcByzA3vMP7dP0PKh5ixBRM6mZryuctu7KAE+ywVTOwYxjZg30tfzSpNEnCaur5zu5pGEpQBZhlAaLvSd1Xmx6JCOt0SvbZ3SXwqAeRHGXRzaTP2MA93lpt1fK06Zalsb2Zx+gIT8yNtY+WKNTPKv/DoWB6mvkdlFPTGocWCjvzcoo5zO7UuF7u+dh X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2017 02:24:05.5783 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR12MB0155 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/12/17 2:53 PM, Borislav Petkov wrote: ... > Ok, a couple of things here: > > * Move the checks first and the allocations second so that you allocate > memory only after all checks have been passed and you don't allocate > pointlessly. I assume you mean performing the SEV state check before allocating the memory for the CSR blob, right ? In my patches, I typically perform all the SW specific checks and allocation before invoking the HW routines. Handling the PSP commands will take longer compare to kmalloc() or access_ok() etc. If its not a big deal then I would prefer to keep that way. > > * That: > > if (state == SEV_STATE_WORKING) { > ret = -EBUSY; > goto e_free_blob; > } else if (state == SEV_STATE_UNINIT) { > ret = sev_firmware_init(&argp->error); > if (ret) > goto e_free_blob; > do_shutdown = 1; > } > > is a repeating pattern. Perhaps it should be called > sev_firmware_reinit() and called by other functions. > * The rest is simplifications and streamlining. > > --- > diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c > index e3ee68afd068..d41f5448a25b 100644 > --- a/drivers/crypto/ccp/psp-dev.c > +++ b/drivers/crypto/ccp/psp-dev.c > @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp) > int ret, state; > void *blob; > > - if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data, > - sizeof(struct sev_user_data_pek_csr))) > + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input))) > + return -EFAULT; > + > + if (!input.address) > + return -EINVAL; > + > + /* allocate a physically contiguous buffer to store the CSR blob */ > + if (!access_ok(VERIFY_WRITE, input.address, input.length) || > + input.length > SEV_FW_BLOB_MAX_SIZE) > return -EFAULT; > > data = kzalloc(sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > - /* allocate a temporary physical contigous buffer to store the CSR blob */ > - blob = NULL; > - if (input.address) { > - if (!access_ok(VERIFY_WRITE, input.address, input.length) || > - input.length > SEV_FW_BLOB_MAX_SIZE) { > - ret = -EFAULT; > - goto e_free; > - } > - > - blob = kmalloc(input.length, GFP_KERNEL); > - if (!blob) { > - ret = -ENOMEM; > - goto e_free; > - } > - > - data->address = __psp_pa(blob); > - data->len = input.length; > + blob = kmalloc(input.length, GFP_KERNEL); > + if (!blob) { > + ret = -ENOMEM; > + goto e_free; > } > > + data->address = __psp_pa(blob); > + data->len = input.length; > + > ret = sev_platform_get_state(&state, &argp->error); > if (ret) > goto e_free_blob; > @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp) > do_shutdown = 1; > } > > - ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error); > + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error); > > input.length = data->len; > > /* copy blob to userspace */ > - if (blob && > - copy_to_user((void __user *)(uintptr_t)input.address, > - blob, input.length)) { > + if (copy_to_user((void __user *)input.address, blob, input.length)) { > ret = -EFAULT; > goto e_shutdown; > } > > - if (copy_to_user((void __user *)(uintptr_t)argp->data, &input, > - sizeof(struct sev_user_data_pek_csr))) > + if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) > ret = -EFAULT; > > e_shutdown: > if (do_shutdown) > - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL); > + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL); > + > e_free_blob: > kfree(blob); > e_free: > @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) > ret = sev_ioctl_pdh_gen(&input); > break; > > - case SEV_PEK_CSR: { > + case SEV_PEK_CSR: > ret = sev_ioctl_pek_csr(&input); > break; > - } > + > default: > ret = -EINVAL; > goto out; >