[PATCH] arch:x86:coco:sev: Initialize ctxt variable

[PATCH] arch:x86:coco:sev: Initialize ctxt variable

[Ragavendra]

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
Signed-off-by: Ragavendra Nagraj <ragavendra.bn@gmail.com>
---
 arch/x86/coco/sev/shared.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
index 71de53194089..a0fe7fc9bdc7 100644
--- a/arch/x86/coco/sev/shared.c
+++ b/arch/x86/coco/sev/shared.c
@@ -335,7 +335,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 = NULL;
 	u8 pending = 0;
 
 	vc_ghcb_invalidate(ghcb);
-- 
2.46.1

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

[Ingo Molnar]

* Ragavendra <ragavendra.bn@gmail.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.

Thanks,

	Ingo

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

[Ard Biesheuvel]

On Fri, 15 Nov 2024 at 12:01, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ragavendra <ragavendra.bn@gmail.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.

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

[Ragavendra B.N.]

On Fri, Nov 15, 2024 at 12:02:27PM +0100, Ard Biesheuvel wrote:
> On Fri, 15 Nov 2024 at 12:01, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Ragavendra <ragavendra.bn@gmail.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.

--
Thanks,
Ragavendra

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

[Ard Biesheuvel]

On Fri, 15 Nov 2024 at 20:53, Ragavendra B.N. <ragavendra.bn@gmail.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@kernel.org> wrote:
> > >
> > >
> > > * Ragavendra <ragavendra.bn@gmail.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?

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

[Tom Lendacky]

On 11/15/24 16:55, Ard Biesheuvel wrote:
> On Fri, 15 Nov 2024 at 20:53, Ragavendra B.N. <ragavendra.bn@gmail.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@kernel.org> wrote:
>>>>
>>>>
>>>> * Ragavendra <ragavendra.bn@gmail.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 = {};

Thanks,
Tom

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

[Tom Lendacky]

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@gmail.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@kernel.org> wrote:
>>>>>
>>>>>
>>>>> * Ragavendra <ragavendra.bn@gmail.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

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

[Ragavendra B.N.]

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@gmail.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@kernel.org> wrote:
> >>>>>
> >>>>>
> >>>>> * Ragavendra <ragavendra.bn@gmail.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;

	}

return ES_EXCEPTION;

Thanks,
Ragavendra N.

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

[Tom Lendacky]

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@gmail.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@kernel.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>> * Ragavendra <ragavendra.bn@gmail.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.

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

[Ragavendra B.N.]

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@gmail.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@kernel.org> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> * Ragavendra <ragavendra.bn@gmail.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.


Thanks,
Ragavendra N.

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

[Tom Lendacky]

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@gmail.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@kernel.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> * Ragavendra <ragavendra.bn@gmail.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.

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

[Ragavendra B.N.]

On Mon, Nov 18, 2024 at 02:37:58PM -0600, Tom Lendacky wrote:
> 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@gmail.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@kernel.org> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> * Ragavendra <ragavendra.bn@gmail.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.

Sure Tom, I will certainly check if I can build correctly and read the suggested documentation as well before sending my patch.


Thanks & regards,
Ragavendra

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

[Ragavendra B.N.]

On Fri, Nov 15, 2024 at 12:01:03PM +0100, Ingo Molnar wrote:
> 
> * Ragavendra <ragavendra.bn@gmail.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.
> 
> Thanks,
> 
> 	Ingo
Please feel free to suggest the valid tag as the file was renamed and I am not able to fetch the correct commit id.
git log --oneline arch/x86/coco/sev/shared.c
f50cd034d24d (HEAD -> 1594023) arch:x86:coco:sev: Initialize ctxt variable
2e1b3cc9d7f7 (grafted) Merge tag 'arm-fixes-6.12-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc

--
Thanks,
Ragavendra

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

[Tom Lendacky]

On 11/15/24 12:37, Ragavendra B.N. wrote:
> On Fri, Nov 15, 2024 at 12:01:03PM +0100, Ingo Molnar wrote:
>>
>> * Ragavendra <ragavendra.bn@gmail.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.
>>
>> Thanks,
>>
>> 	Ingo
> Please feel free to suggest the valid tag as the file was renamed and I am not able to fetch the correct commit id.
> git log --oneline arch/x86/coco/sev/shared.c
> f50cd034d24d (HEAD -> 1594023) arch:x86:coco:sev: Initialize ctxt variable
> 2e1b3cc9d7f7 (grafted) Merge tag 'arm-fixes-6.12-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc

A quick git annotate arch/x86/coco/sev/shared.c shows that function was
created with:

34ff65901735 ("x86/sev: Use kernel provided SVSM Calling Areas")

Thanks,
Tom

> 
> --
> Thanks,
> Ragavendra

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

[Tom Lendacky]

On 11/15/24 13:20, Tom Lendacky wrote:
> On 11/15/24 12:37, Ragavendra B.N. wrote:
>> On Fri, Nov 15, 2024 at 12:01:03PM +0100, Ingo Molnar wrote:
>>>
>>> * Ragavendra <ragavendra.bn@gmail.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.
>>>
>>> Thanks,
>>>
>>> 	Ingo
>> Please feel free to suggest the valid tag as the file was renamed and I am not able to fetch the correct commit id.
>> git log --oneline arch/x86/coco/sev/shared.c
>> f50cd034d24d (HEAD -> 1594023) arch:x86:coco:sev: Initialize ctxt variable
>> 2e1b3cc9d7f7 (grafted) Merge tag 'arm-fixes-6.12-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
> 
> A quick git annotate arch/x86/coco/sev/shared.c shows that function was
> created with:
> 
> 34ff65901735 ("x86/sev: Use kernel provided SVSM Calling Areas")

Also, the subject line should be:

	x86/sev: Initialize ctxt variable

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> --
>> Thanks,
>> Ragavendra

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

[Ragavendra B.N.]

On Fri, Nov 15, 2024 at 01:20:15PM -0600, Tom Lendacky wrote:
> On 11/15/24 12:37, Ragavendra B.N. wrote:
> > On Fri, Nov 15, 2024 at 12:01:03PM +0100, Ingo Molnar wrote:
> >>
> >> * Ragavendra <ragavendra.bn@gmail.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.
> >>
> >> Thanks,
> >>
> >> 	Ingo
> > Please feel free to suggest the valid tag as the file was renamed and I am not able to fetch the correct commit id.
> > git log --oneline arch/x86/coco/sev/shared.c
> > f50cd034d24d (HEAD -> 1594023) arch:x86:coco:sev: Initialize ctxt variable
> > 2e1b3cc9d7f7 (grafted) Merge tag 'arm-fixes-6.12-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
> 
> A quick git annotate arch/x86/coco/sev/shared.c shows that function was
> created with:
> 
> 34ff65901735 ("x86/sev: Use kernel provided SVSM Calling Areas")
> 
> Thanks,
> Tom
> 
> > 
> > --
> > Thanks,
> > Ragavendra
Thanks a lot Tom.

I figured why I ran into it in the first place. When I cloned, I cloned with --depth 1 as I had less free space in that partition. The annotate flag certainly helped to retrieve the exact commit id.

--
Regards,
Ragavendra N
,