Re: [PATCH] KVM: SVM: Fix "error" isn't initialized

Re: [PATCH] KVM: SVM: Fix "error" isn't initialized

[linmiaohe]

> From: Haiwei Li <lihaiwei@tencent.com>
> Subject: [PATCH] initialize 'error'
>
> There are a bunch of error paths were "error" isn't initialized.
Hi,
In case error case, sev_guest_df_flush() do not set the error.
Can you set the value of error to reflect what error happened
in sev_guest_df_flush()?
The current fix may looks confused when print "DF_FLUSH failed" with
error = 0.
Thanks. 

PS: This is just my personal point.
>
> Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>--- a/arch/x86/kvm/svm.c
>+++ b/arch/x86/kvm/svm.c
>@@ -6294,7 +6294,8 @@ static int enable_smi_window(struct kvm_vcpu *vcpu)
>
> static int sev_flush_asids(void)
> {
>-	int ret, error;
>+	int ret;
>+	int error = 0;
>
> 	/*
> 	 * DEACTIVATE will clear the WBINVD indicator causing DF_FLUSH to fail,

Re: [PATCH] KVM: SVM: Fix "error" isn't initialized

[Sean Christopherson]

On Wed, Nov 27, 2019 at 03:30:06AM +0000, linmiaohe wrote:
> 
> > From: Haiwei Li <lihaiwei@tencent.com>
> > Subject: [PATCH] initialize 'error'
> >
> > There are a bunch of error paths were "error" isn't initialized.
> Hi,
> In case error case, sev_guest_df_flush() do not set the error.
> Can you set the value of error to reflect what error happened
> in sev_guest_df_flush()?
> The current fix may looks confused when print "DF_FLUSH failed" with
> error = 0.
> Thanks. 
> 
> PS: This is just my personal point.

Disclaimer: not my world at all...

Based on the prototype for __sev_do_cmd_locked(), @error is intended to be
filled only if there's an actual response from the PSP, which is a 16-bit
value.  So maybe init @psp_ret at the beginning of __sev_do_cmd_locked() to
-1 to indicate the command was never sent to the PSP?  And update the
pr_err() in sev_asid_flush() to explicitly state it's the PSP return?

Re: [PATCH] KVM: SVM: Fix "error" isn't initialized

[Haiwei Li]

On 2019/11/27 11:44, Sean Christopherson wrote:
> On Wed, Nov 27, 2019 at 03:30:06AM +0000, linmiaohe wrote:
>>
>>> From: Haiwei Li <lihaiwei@tencent.com>
>>> Subject: [PATCH] initialize 'error'
>>>
>>> There are a bunch of error paths were "error" isn't initialized.
>> Hi,
>> In case error case, sev_guest_df_flush() do not set the error.
>> Can you set the value of error to reflect what error happened
>> in sev_guest_df_flush()?
>> The current fix may looks confused when print "DF_FLUSH failed" with
>> error = 0.
>> Thanks.
>>
>> PS: This is just my personal point.
> 
> Disclaimer: not my world at all...
> 
> Based on the prototype for __sev_do_cmd_locked(), @error is intended to be
> filled only if there's an actual response from the PSP, which is a 16-bit
> value.  So maybe init @psp_ret at the beginning of __sev_do_cmd_locked() to
> -1 to indicate the command was never sent to the PSP?  And update the
> pr_err() in sev_asid_flush() to explicitly state it's the PSP return?
> 

Thanks for your advise. Good point. I will send a new patch.

Re: [PATCH] KVM: SVM: Fix "error" isn't initialized

[linmiaohe]

On 2019/11/27 11:44, Sean Christopherson wrote:
> On Wed, Nov 27, 2019 at 03:30:06AM +0000, linmiaohe wrote:
>>
>>> From: Haiwei Li <lihaiwei@tencent.com>
>>> Subject: [PATCH] initialize 'error'
>>>
>>> There are a bunch of error paths were "error" isn't initialized.
>> Hi,
>> In case error case, sev_guest_df_flush() do not set the error.
>> Can you set the value of error to reflect what error happened in 
>> sev_guest_df_flush()?
>> The current fix may looks confused when print "DF_FLUSH failed" with 
>> error = 0.
>> Thanks.
>>
>> PS: This is just my personal point.
> 
> Disclaimer: not my world at all...
> 
> Based on the prototype for __sev_do_cmd_locked(), @error is intended 
> to be filled only if there's an actual response from the PSP, which is 
> a 16-bit value.  So maybe init @psp_ret at the beginning of 
> __sev_do_cmd_locked() to
> -1 to indicate the command was never sent to the PSP?  And update the
> pr_err() in sev_asid_flush() to explicitly state it's the PSP return?
>

I think it's a good alternative. Many Thanks.

[PATCH] KVM: SVM: Fix "error" isn't initialized

[Haiwei Li]

 From d32ebcf6f426385942fe6c469255e73188cd7d38 Mon Sep 17 00:00:00 2001
From: Haiwei Li <lihaiwei@tencent.com>
Date: Wed, 27 Nov 2019 11:03:21 +0800
Subject: [PATCH] initialize 'error'

There are a bunch of error paths were "error" isn't initialized.

Signed-off-by: Haiwei Li <lihaiwei@tencent.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
  arch/x86/kvm/svm.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 362e874..0b3d49c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6294,7 +6294,8 @@ static int enable_smi_window(struct kvm_vcpu *vcpu)

  static int sev_flush_asids(void)
  {
-	int ret, error;
+	int ret;
+	int error = 0;

  	/*
  	 * DEACTIVATE will clear the WBINVD indicator causing DF_FLUSH to fail,
--
1.8.3.1