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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b3b0f19b-b138-b386-ece0-a1f14a1e2f66@amd.com>
Date: Tue, 19 Nov 2024 13:11:16 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: "Ragavendra B.N." <ragavendra.bn@...il.com>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, ardb@...nel.org,
 tzimmermann@...e.de, bhelgaas@...gle.com, x86@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/sev: Initialize ctxt variable and zero fi

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@...il.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