x86/sev: Initialize ctxt variable and zero fi

[PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Ragavendra 5 days, 1 hour ago

Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
it was not initialized. Updating memory to zero for the ctxt->fi
variable in verify_exception_info when ES_EXCEPTION is returned.

Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
---
 arch/x86/coco/sev/shared.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
index 71de53194089..5e0f6fbf4dd2 100644
--- a/arch/x86/coco/sev/shared.c
+++ b/arch/x86/coco/sev/shared.c
@@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
 		if ((info & SVM_EVTINJ_VALID) &&
 		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
 		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
+			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
+
 			ctxt->fi.vector = v;
 
 			if (info & SVM_EVTINJ_VALID_ERR)
@@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
 
 static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
 {
-	struct es_em_ctxt ctxt;
+	struct es_em_ctxt ctxt = {};
 	u8 pending = 0;
 
 	vc_ghcb_invalidate(ghcb);
-- 
2.46.1

Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Tom Lendacky 4 days, 9 hours ago

On 11/18/24 16:58, Ragavendra wrote:
> Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
> it was not initialized. Updating memory to zero for the ctxt->fi
> variable in verify_exception_info when ES_EXCEPTION is returned.
> 
> Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
> Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
> ---
>  arch/x86/coco/sev/shared.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
> index 71de53194089..5e0f6fbf4dd2 100644
> --- a/arch/x86/coco/sev/shared.c
> +++ b/arch/x86/coco/sev/shared.c
> @@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
>  		if ((info & SVM_EVTINJ_VALID) &&
>  		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
>  		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
> +			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
> +
>  			ctxt->fi.vector = v;
>  
>  			if (info & SVM_EVTINJ_VALID_ERR)
> @@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
>  
>  static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
>  {
> -	struct es_em_ctxt ctxt;
> +	struct es_em_ctxt ctxt = {};

This isn't necessary if you are doing the memset.

Thanks,
Tom

>  	u8 pending = 0;
>  
>  	vc_ghcb_invalidate(ghcb);

Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Ragavendra B.N. 4 days, 6 hours ago

On Tue, Nov 19, 2024 at 08:23:14AM -0600, Tom Lendacky wrote:
> On 11/18/24 16:58, Ragavendra wrote:
> > Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
> > it was not initialized. Updating memory to zero for the ctxt->fi
> > variable in verify_exception_info when ES_EXCEPTION is returned.
> > 
> > Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
> > Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
> > ---
> >  arch/x86/coco/sev/shared.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
> > index 71de53194089..5e0f6fbf4dd2 100644
> > --- a/arch/x86/coco/sev/shared.c
> > +++ b/arch/x86/coco/sev/shared.c
> > @@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
> >  		if ((info & SVM_EVTINJ_VALID) &&
> >  		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
> >  		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
> > +			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
> > +
> >  			ctxt->fi.vector = v;
> >  
> >  			if (info & SVM_EVTINJ_VALID_ERR)
> > @@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
> >  
> >  static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
> >  {
> > -	struct es_em_ctxt ctxt;
> > +	struct es_em_ctxt ctxt = {};
> 
> This isn't necessary if you are doing the memset.
> 
> Thanks,
> Tom
> 
> >  	u8 pending = 0;
> >  
> >  	vc_ghcb_invalidate(ghcb);

I can go ahead and undo that, I fear that Coverity can catch it. If no harm I can leave it.


--
Thanks & regards,
Ragavendra N

Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Tom Lendacky 4 days, 6 hours ago

On 11/19/24 11:35, Ragavendra B.N. wrote:
> On Tue, Nov 19, 2024 at 08:23:14AM -0600, Tom Lendacky wrote:
>> On 11/18/24 16:58, Ragavendra wrote:
>>> Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
>>> it was not initialized. Updating memory to zero for the ctxt->fi
>>> variable in verify_exception_info when ES_EXCEPTION is returned.
>>>
>>> Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
>>> Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
>>> ---
>>>  arch/x86/coco/sev/shared.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
>>> index 71de53194089..5e0f6fbf4dd2 100644
>>> --- a/arch/x86/coco/sev/shared.c
>>> +++ b/arch/x86/coco/sev/shared.c
>>> @@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
>>>  		if ((info & SVM_EVTINJ_VALID) &&
>>>  		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
>>>  		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
>>> +			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
>>> +
>>>  			ctxt->fi.vector = v;
>>>  
>>>  			if (info & SVM_EVTINJ_VALID_ERR)
>>> @@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
>>>  
>>>  static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
>>>  {
>>> -	struct es_em_ctxt ctxt;
>>> +	struct es_em_ctxt ctxt = {};
>>
>> This isn't necessary if you are doing the memset.
>>
>> Thanks,
>> Tom
>>
>>>  	u8 pending = 0;
>>>  
>>>  	vc_ghcb_invalidate(ghcb);
> 
> I can go ahead and undo that, I fear that Coverity can catch it. If no harm I can leave it.

Well, can you remove the line and run Coverity and see if it still
thinks there's an issue?

If it sees an issue, then it could be that Coverity can't follow the
flow completely in this case. Doing the memset is enough, as far as I
can see.

Thanks,
Tom

> 
> 
> --
> Thanks & regards,
> Ragavendra N

Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Ragavendra B.N. 4 days, 6 hours ago

On Tue, Nov 19, 2024 at 11:51:27AM -0600, Tom Lendacky wrote:
> On 11/19/24 11:35, Ragavendra B.N. wrote:
> > On Tue, Nov 19, 2024 at 08:23:14AM -0600, Tom Lendacky wrote:
> >> On 11/18/24 16:58, Ragavendra wrote:
> >>> Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
> >>> it was not initialized. Updating memory to zero for the ctxt->fi
> >>> variable in verify_exception_info when ES_EXCEPTION is returned.
> >>>
> >>> Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
> >>> Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
> >>> ---
> >>>  arch/x86/coco/sev/shared.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
> >>> index 71de53194089..5e0f6fbf4dd2 100644
> >>> --- a/arch/x86/coco/sev/shared.c
> >>> +++ b/arch/x86/coco/sev/shared.c
> >>> @@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
> >>>  		if ((info & SVM_EVTINJ_VALID) &&
> >>>  		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
> >>>  		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
> >>> +			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
> >>> +
> >>>  			ctxt->fi.vector = v;
> >>>  
> >>>  			if (info & SVM_EVTINJ_VALID_ERR)
> >>> @@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
> >>>  
> >>>  static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
> >>>  {
> >>> -	struct es_em_ctxt ctxt;
> >>> +	struct es_em_ctxt ctxt = {};
> >>
> >> This isn't necessary if you are doing the memset.
> >>
> >> Thanks,
> >> Tom
> >>
> >>>  	u8 pending = 0;
> >>>  
> >>>  	vc_ghcb_invalidate(ghcb);
> > 
> > I can go ahead and undo that, I fear that Coverity can catch it. If no harm I can leave it.
> 
> Well, can you remove the line and run Coverity and see if it still
> thinks there's an issue?
> 
> If it sees an issue, then it could be that Coverity can't follow the
> flow completely in this case. Doing the memset is enough, as far as I
> can see.
> 
> Thanks,
> Tom
> 
> > 
> > 
> > --
> > Thanks & regards,
> > Ragavendra N

Sure Tom, I have updated the change and sent the new patch. Please let me know if everything looks fine,


Regards,
Ragavendra N

Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

Posted by Tom Lendacky 4 days, 5 hours ago

On 11/19/24 12:08, Ragavendra B.N. wrote:
> On Tue, Nov 19, 2024 at 11:51:27AM -0600, Tom Lendacky wrote:
>> On 11/19/24 11:35, Ragavendra B.N. wrote:
>>> On Tue, Nov 19, 2024 at 08:23:14AM -0600, Tom Lendacky wrote:
>>>> On 11/18/24 16:58, Ragavendra wrote:
>>>>> Updating the ctxt value to {} in the svsm_perform_ghcb_protocol as
>>>>> it was not initialized. Updating memory to zero for the ctxt->fi
>>>>> variable in verify_exception_info when ES_EXCEPTION is returned.
>>>>>
>>>>> Fixes: 34ff65901735 x86/sev: Use kernel provided SVSM Calling Areas
>>>>> Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
>>>>> ---
>>>>>  arch/x86/coco/sev/shared.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
>>>>> index 71de53194089..5e0f6fbf4dd2 100644
>>>>> --- a/arch/x86/coco/sev/shared.c
>>>>> +++ b/arch/x86/coco/sev/shared.c
>>>>> @@ -239,6 +239,8 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt
>>>>>  		if ((info & SVM_EVTINJ_VALID) &&
>>>>>  		    ((v == X86_TRAP_GP) || (v == X86_TRAP_UD)) &&
>>>>>  		    ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
>>>>> +			memset(&ctxt->fi, 0, sizeof(ctxt->fi));
>>>>> +
>>>>>  			ctxt->fi.vector = v;
>>>>>  
>>>>>  			if (info & SVM_EVTINJ_VALID_ERR)
>>>>> @@ -335,7 +337,7 @@ static int svsm_perform_msr_protocol(struct svsm_call *call)
>>>>>  
>>>>>  static int svsm_perform_ghcb_protocol(struct ghcb *ghcb, struct svsm_call *call)
>>>>>  {
>>>>> -	struct es_em_ctxt ctxt;
>>>>> +	struct es_em_ctxt ctxt = {};
>>>>
>>>> This isn't necessary if you are doing the memset.
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>  	u8 pending = 0;
>>>>>  
>>>>>  	vc_ghcb_invalidate(ghcb);
>>>
>>> I can go ahead and undo that, I fear that Coverity can catch it. If no harm I can leave it.
>>
>> Well, can you remove the line and run Coverity and see if it still
>> thinks there's an issue?
>>
>> If it sees an issue, then it could be that Coverity can't follow the
>> flow completely in this case. Doing the memset is enough, as far as I
>> can see.
>>
>> Thanks,
>> Tom
>>
>>>
>>>
>>> --
>>> Thanks & regards,
>>> Ragavendra N
> 
> Sure Tom, I have updated the change and sent the new patch. Please let me know if everything looks fine,

So does that mean you ran it through Coverity and everything was ok?

Thanks,
Tom

> 
> 
> Regards,
> Ragavendra N