linux-kernel - Re: [PATCH] arch:x86:coco:sev: Initialize ctxt variable

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6539edcb-e865-e33e-5344-77fac413e49d@amd.com>
Date: Mon, 18 Nov 2024 14:37:58 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: "Ragavendra B.N." <ragavendra.bn@...il.com>
Cc: Ard Biesheuvel <ardb@...nel.org>, Ingo Molnar <mingo@...nel.org>,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, ashish.kalra@....com,
 tzimmermann@...e.de, bhelgaas@...gle.com, x86@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arch:x86:coco:sev: Initialize ctxt variable

On 11/18/24 14:22, Ragavendra B.N. wrote:
> On Mon, Nov 18, 2024 at 01:50:55PM -0600, Tom Lendacky wrote:
>> On 11/18/24 13:43, Ragavendra B.N. wrote:
>>> On Mon, Nov 18, 2024 at 08:53:04AM -0600, Tom Lendacky wrote:
>>>> On 11/18/24 08:44, Tom Lendacky wrote:
>>>>> On 11/15/24 16:55, Ard Biesheuvel wrote:
>>>>>> On Fri, 15 Nov 2024 at 20:53, Ragavendra B.N. <ragavendra.bn@...il.com> wrote:
>>>>>>>
>>>>>>> On Fri, Nov 15, 2024 at 12:02:27PM +0100, Ard Biesheuvel wrote:
>>>>>>>> On Fri, 15 Nov 2024 at 12:01, Ingo Molnar <mingo@...nel.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> * Ragavendra <ragavendra.bn@...il.com> wrote:
>>>>>>>>>
>>>>>>>>>> Updating the ctxt value to NULL in the svsm_perform_ghcb_protocol as
>>>>>>>>>> it was not initialized.
>>>>>>>>>>
>>>>>>>>>> Fixes: 2e1b3cc9d7f7 (grafted) Merge tag 'arm-fixes-6.12-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
>>>>>>>>>
>>>>>>>>> This 'Fixes' tag looks bogus.
>>>>>>>>>
>>>>>>>>
>>>>>>>> So does the patch itself - 'struct es_em_ctxt ctxt' is not a pointer.
>>>>>>> Thank you very much for your response. I am relatively new to kernel development.
>>>>>>>
>>>>>>> I know we can use kmalloc for memory allocation. Please advice.
>>>>>>>
>>>>>>> struct es_em_ctxt ctxt = kmalloc(sizeof(struct es_em_ctxt), GFP_KERNEL);
>>>>>>>
>>>>>>> I am thinking to update like above, but like you mentioned, ctxt is not a pointer. I can update this to be a pointer if needed.
>>>>>>>
>>>>>>
>>>>>> The code is fine as is. Let's end this thread here, shall we?
>>>>>
>>>>> I was assuming he got some kind of warning from some compiler options or
>>>>> a static checker. Is that the case Ragavendra?
>>>>>
>>>>> When I look at the code, it is possible for ctxt->fi.error_code to be
>>>>> left uninitialized. The simple fix is to just initialize ctxt as:
>>>>>
>>>>> 	struct es_em_ctxt ctxt = {};
>>>>
>>>> Although to cover all cases now and going forwared, the es_em_ctxt fi
>>>> member should just be zeroed in verify_exception_info() when
>>>> ES_EXCEPTION is going to be returned.
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>
>>>>> Thanks,
>>>>> Tom
>>>
>>> Yes Tom, that is exactly the reason I worked on it the first place. The issue was reported by the Coverity tool.
>>>
>>> I can send the below fix if that is fine.
>>>>> 	struct es_em_ctxt ctxt = {};
>>>
>>> For the es_em_ctxt fi member to be zeroed, I can go ahead and assign 0 to all the three long members like below in verify_exception_info()
>>>
>>>
>>> 	if (info & SVM_EVTINJ_VALID_ERR) {
>>> 		ctxt->fi.error_code = info >> 32;
>>> 	} else {
>>> 		ctxt->fi.error_code = 0;
>>> 		ctxt->fi.vector = 0;
>>> 		ctxt->fi.cr2 = 0;
>>
>> But then the cr2 value isn't set/zeroed in the true path of the if
>> statement. I think a simple memset() at the beginning of the if path
>> that will return ES_EXCEPTION is simplest.
>>
>> Thanks,
>> Tom
>>
>>>
>>> 	}
>>>
>>> return ES_EXCEPTION;
>>>
>>> Thanks,
>>> Ragavendra N.
> 
> I am assuming something like below.
> 
> /* Check if exception information from hypervisor is sane. */
> 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(es_fault_info));
> 
> 	ctxt->fi.vector = v;
> 
> PS - My C skills is not that great as well, as I am from Java/ C# background.

Yes, that is the general idea.

Please be sure that whatever you submit builds properly before
submitting. For example, the above will fail to build (as would have
your first patch).

Be sure to read Documentation/process/coding-style.rst and
Documentation/process/submitting-patches.rst.

Thanks,
Tom

> 
> 
> Thanks,
> Ragavendra N.