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;
Message-ID: <518bfa74-ee4a-4539-3310-130c07916300@amd.com>
Date:   Mon, 14 Nov 2022 15:12:59 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v8 3/4] virt: sev-guest: Remove err in
 handle_guest_request
Content-Language: en-US
To:     Dionna Glaze <dionnaglaze@google.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Joerg Roedel <jroedel@suse.de>,
        Peter Gonda <pgonda@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@suse.de>, Haowen Bai <baihaowen@meizu.com>,
        Liam Merwick <liam.merwick@oracle.com>,
        Yang Yingliang <yangyingliang@huawei.com>
References: <20221104230040.2346862-1-dionnaglaze@google.com>
 <20221104230040.2346862-4-dionnaglaze@google.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20221104230040.2346862-4-dionnaglaze@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NVptS1BhRTcrVkFXcFhmdjgyWWF1Ujh5Y0FLdTA4V3NpMHNRWkRMR3pMV3VF?=
 =?utf-8?B?TVFOc3FxL2JQazZaZ2hZN3RDSWVLOGJ6M2hYOTJyVENDN0RYR3NRYnBtMDgw?=
 =?utf-8?B?N1BUTXNya1N3RVB3QXJ3eVd6bCtDbGVOdnNGc3NYajVhQ0ljWk1iWS9FZ1Jq?=
 =?utf-8?B?U2xRM0FPZDlvTkd4WTNCMDhPanpLNUVMdGk4bmhaQXl2WUZ6c3FzeFBoU2h6?=
 =?utf-8?B?YTlmdkJnLzNldWhnVkRaZ0NTb0kyQTNDNXhwVXB1ZktPQXlCUXFpeGQrNTdh?=
 =?utf-8?B?NFE3TXFuR0J3QjlqMG1GS1M0MENIaUc2WkQ2NTVEK3RrQWFsZWFNN2U1TElu?=
 =?utf-8?B?THVZdmRlYVhlSDJ4V2dYQWZjTlBpenFaQURvNVNoVmhialFLRHBXbzhiT0Zq?=
 =?utf-8?B?L3dEa2tDbjhwYTRXbkpaZnhoTzFsSUhxRjVjQkdnTnRKT1dzeTNDY24xaVpY?=
 =?utf-8?B?Z1R4RXgybERQbGQvOU1DR1N5cGY3Z1plMHhWWVJkNjR3K2paWGF3clNobVR6?=
 =?utf-8?B?NE40d1oxVXlGeGxqNGNybHJnd1Y5QmJxZHFvQlU1MWRRVDVEM2hNRWNVYUlW?=
 =?utf-8?B?eWZXdjJJaDBWVDJyQm00ZzJjN2huZHcza2dPOU95WVB2aUJoTW0rRDZ5dUVq?=
 =?utf-8?B?dy9qdXdmc01aSW9GM1dHWkNUL1F5Q1lqUHdXTDI3dHdyK1Y4UzZSK1RhVHZt?=
 =?utf-8?B?NnYvMEs3c1FUdkVVcjFEcnBRa09PRDVIcXBpdE1JdGN4UEl1WFNEWHBlbW9M?=
 =?utf-8?B?K3RaaWYxK0k0c3U3Y1RyNjN1cUdhM0ZIT1NDQld0eEdLcUFBVkx1UUNzMDhi?=
 =?utf-8?B?VVE1Tm1VSnhrdHJnR2NuTXRYODB4MThxUEthcGR3OU4zWFJhSG91ajdmMUlp?=
 =?utf-8?B?T3A1WHNEbTM3UmZpcTd0TFdKekFVTFhhSnNobExnOHQzbGphVTNFMnJVOGRo?=
 =?utf-8?B?Q0hOWXMzekViY2NwY3hsVDFDb2xiUTB5Z2F6clA5WlJsZE95Y1dMbFJKdzRF?=
 =?utf-8?B?b0liL0s3YzJOQ25OaGRXeElGWEFXVTd5M3V6SStmTWN6N3pxdWhKVGRzNXQv?=
 =?utf-8?B?SklmSVQ2ZUl3Wm9GeVpyaXBkOGEzYTI0WXBmSVRvTnpuVGMwNmRmcEN3aDNu?=
 =?utf-8?B?UENUTWp3clBqbWpUR2JVdjdpMUdKQXo0U01May9kZmpIeHBWR0Y0dWF3T1BF?=
 =?utf-8?B?ajA1dzNSSUJZOUphd25EekVqRTFMczFHT0VuV2gvYXdXTXVLQnRHS2luZUJI?=
 =?utf-8?B?ZXZ4OVlFbjdJbEpRL3c1SWdJSmFlWFI3N2ZMZlYrWWlHL0lmcG02ZTN5T2hk?=
 =?utf-8?B?Rm00YmE1UzNvbmN5eVVSZi8wdm1IdkxEbTYwZENId2VSb2g0ZjFPOUYydWlk?=
 =?utf-8?B?VFVma3ovbFZoVVYyV2hEbDFqR2xxak9hdFdxMW9jWUEwZmdUYXFKbDF0TTZr?=
 =?utf-8?B?dDJwai9RMmZYK09Xa0Y2OXJVVllqdG5VWUx4RkkwV0tOQVVubUk4eGpscDBU?=
 =?utf-8?B?M2ZPOWdFdWVVamMvSVF5OVEvUENSWGtBODFVZEtGaVBVQUQ3dzJ0azNHZWpQ?=
 =?utf-8?B?SHVSeXN2QUt0UlE1T0ZsSm5DZHkxTFZCdFJxenhOWitsZW52bkN2dkNtT0xQ?=
 =?utf-8?B?bVBMOHV5R2pacDZtTmFDaU5RT0hzLzNlZGhNdG1za2lLUFUrNXZzRmhjUkI0?=
 =?utf-8?B?TmZJUjBlckNYVEE1M1ZBaVNLQ0IvQldGKy95Yy9NTFROVlV3a3ByRWdpQy9j?=
 =?utf-8?B?dnk4bWNFbldocWpkUlJiZEMydTdFTE1VYjg4RUVLM0FFSVR3QWpzL1V2ZEZm?=
 =?utf-8?B?c1hpUzltMHdnSFFQajBZQ0pjRHFjbFJITFIwWWYzQS8zc2tsQ0ZHL05PSkRj?=
 =?utf-8?B?bUF3M213cGRQYVV0QytNRjAyWnFPMlJDUWowdlNac3lmRXFMaEtCczJ2SmVF?=
 =?utf-8?B?aG9WeEpNdTlzTjdqZXJORk1vRmRySTYxY2JtUnZlWkQ1TTl5akhZL2p3QlB1?=
 =?utf-8?B?MDlLYWYxZndreU13T1Btbjdka0svNklIV1lFbS9UTW5TRDNQdXN6aXJYYkY5?=
 =?utf-8?B?V1NCM1grVWxqSEc3bEdPcHA4eGlRcWhxOGt4YlZSZm10aWtxbnN6SmRBVEtl?=
 =?utf-8?Q?bSR7LhlToXB9a4oa9+Ky3mPJf?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f54a42b-fab0-443a-9e62-08dac6850362
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Nov 2022 21:13:01.7185
 (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: 8LDt5o33b32sIpRhGcQuYlwMWao9oy3aYV9k+hYQXzaveO7Nuu/YfiIawbavsL+GsU3ek6K075RZbedZVl0HDQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB5725
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,
        SPF_PASS 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

On 11/4/22 18:00, Dionna Glaze wrote:
> The err variable may not be set in the call to snp_issue_guest_request,
> yet it is unconditionally written back to fw_err if fw_err is non-null.
> This is undefined behavior, and currently returns uninitialized kernel
> stack memory to user space.
> 
> The fw_err argument is better to just pass through to
> snp_issue_guest_request, so we do that by passing along the ioctl
> argument. This removes the need for an argument to handle_guest_request.
> 
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Haowen Bai <baihaowen@meizu.com>
> Cc: Liam Merwick <liam.merwick@oracle.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> 
> Fixes: fce96cf04430 ("virt: Add SEV-SNP guest driver")
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   drivers/virt/coco/sev-guest/sev-guest.c | 37 ++++++++++++-------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index d08ff87c2dac..5615d349b378 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -320,11 +320,11 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8
>   	return __enc_payload(snp_dev, req, payload, sz);
>   }
>   
> -static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, int msg_ver,
> +static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> +				struct snp_guest_request_ioctl *arg,
>   				u8 type, void *req_buf, size_t req_sz, void *resp_buf,
> -				u32 resp_sz, __u64 *fw_err)
> +				u32 resp_sz)
>   {
> -	unsigned long err;
>   	u64 seqno;
>   	int rc;
>   
> @@ -336,12 +336,14 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>   	memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
>   
>   	/* Encrypt the userspace provided payload */
> -	rc = enc_payload(snp_dev, seqno, msg_ver, type, req_buf, req_sz);
> +	rc = enc_payload(snp_dev, seqno, arg->msg_version, type, req_buf,
> +			 req_sz);
>   	if (rc)
>   		return rc;
>   
>   	/* Call firmware to process the request */
> -	rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> +	rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> +				     &arg->fw_err);
>   
>   	/*
>   	 * If the extended guest request fails due to having to small of a
> @@ -349,23 +351,21 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>   	 * extended data request.
>   	 */
>   	if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
> -		err == SNP_GUEST_REQ_INVALID_LEN) {
> +		arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
>   		const unsigned int certs_npages = snp_dev->input.data_npages;
>   
>   		exit_code = SVM_VMGEXIT_GUEST_REQUEST;
> -		rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> +		rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> +					     &arg->fw_err);
>   
> -		err = SNP_GUEST_REQ_INVALID_LEN;
> +		arg->fw_err = SNP_GUEST_REQ_INVALID_LEN;
>   		snp_dev->input.data_npages = certs_npages;
>   	}
>   
> -	if (fw_err)
> -		*fw_err = err;
> -
>   	if (rc) {
>   		dev_alert(snp_dev->dev,
>   			  "Detected error from ASP request. rc: %d, fw_err: %llu\n",
> -			  rc, *fw_err);
> +			  rc, arg->fw_err);
>   		goto disable_vmpck;
>   	}
>   
> @@ -412,9 +412,9 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
>   	if (!resp)
>   		return -ENOMEM;
>   
> -	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version,
> +	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
>   				  SNP_MSG_REPORT_REQ, &req, sizeof(req), resp->data,
> -				  resp_len, &arg->fw_err);
> +				  resp_len);
>   	if (rc)
>   		goto e_free;
>   
> @@ -452,9 +452,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
>   	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
>   		return -EFAULT;
>   
> -	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version,
> -				  SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len,
> -				  &arg->fw_err);
> +	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
> +				  SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len);
>   	if (rc)
>   		return rc;
>   
> @@ -514,9 +513,9 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   		return -ENOMEM;
>   
>   	snp_dev->input.data_npages = npages;
> -	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version,
> +	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
>   				   SNP_MSG_REPORT_REQ, &req.data,
> -				   sizeof(req.data), resp->data, resp_len, &arg->fw_err);
> +				   sizeof(req.data), resp->data, resp_len);
>   
>   	/* If certs length is invalid then copy the returned length */
>   	if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {