Updating es_em_ctxt fi to zero

[PATCH] Updating es_em_ctxt fi to zero

Posted by Ragavendra 1 year, 2 months ago

Updating es_em_ctxt 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
index 71de53194089..b8540d85e6f0 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)
-- 
2.46.1

Re: [PATCH] Updating es_em_ctxt fi to zero

Posted by Bjorn Helgaas 1 year, 2 months ago

On Tue, Nov 19, 2024 at 10:05:18AM -0800, Ragavendra wrote:
> Updating es_em_ctxt to zero for the ctxt->fi variable in
> verify_exception_info when ES_EXCEPTION is returned.

This commit log basically says in English what the code does in C.  If
you can include the *reason* why this is important, it will be more
helpful.  For example, maybe somebody consumes other parts of ctxt.fi
(a struct es_fault_info), and without this patch, they use junk that
causes an oops or some other bad thing.

If the 34ff65901735 Fixes: tag is correct, I suppose the problem
happens because ctxt is allocated on the stack and contains junk, and
then svsm_perform_ghcb_protocol() passes it on to
vc_forward_exception(), which does use fields of ctxt->fi other than
.vector, which will be junk without this patch.

Hints and samples for commit logs:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v6.11#n134

Based on "git log --oneline arch/x86/coco/sev", I would expect the
subject line to have an "x86/sev: " prefix, e.g.,

  x86/sev: Clear es_em_ctxt.fi to ...

> 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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
> index 71de53194089..b8540d85e6f0 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)
> -- 
> 2.46.1
>

Re: [PATCH] Updating es_em_ctxt fi to zero

Posted by Ragavendra B.N. 1 year, 2 months ago

On Tue, Nov 19, 2024 at 01:26:02PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 10:05:18AM -0800, Ragavendra wrote:
> > Updating es_em_ctxt to zero for the ctxt->fi variable in
> > verify_exception_info when ES_EXCEPTION is returned.
> 
> This commit log basically says in English what the code does in C.  If
> you can include the *reason* why this is important, it will be more
> helpful.  For example, maybe somebody consumes other parts of ctxt.fi
> (a struct es_fault_info), and without this patch, they use junk that
> causes an oops or some other bad thing.
> 
> If the 34ff65901735 Fixes: tag is correct, I suppose the problem
> happens because ctxt is allocated on the stack and contains junk, and
> then svsm_perform_ghcb_protocol() passes it on to
> vc_forward_exception(), which does use fields of ctxt->fi other than
> .vector, which will be junk without this patch.
> 
> Hints and samples for commit logs:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v6.11#n134
> 
> Based on "git log --oneline arch/x86/coco/sev", I would expect the
> subject line to have an "x86/sev: " prefix, e.g.,
> 
>   x86/sev: Clear es_em_ctxt.fi to ...
> 
> > 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 | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/coco/sev/shared.c b/arch/x86/coco/sev/shared.c
> > index 71de53194089..b8540d85e6f0 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)
> > -- 
> > 2.46.1
> > 

Yes Bjorn, I completely agree with the need to update the reason, I will update the commit log and send the newer version accordingly.


--
Thanks & regards,
Ragavendra N

Re: [PATCH] Updating es_em_ctxt fi to zero

Posted by Borislav Petkov 1 year, 2 months ago

On Tue, Nov 19, 2024 at 11:46:20AM -0800, Ragavendra B.N. wrote:
> Yes Bjorn, I completely agree with the need to update the reason, I will
> update the commit log and send the newer version accordingly.

I think you should take the time, read that handbook Bjorn pointed you to and
then read 

https://kernel.org/doc/html/latest/process/development-process.html

and especially

https://kernel.org/doc/html/latest/process/submitting-patches.html

before you submit more patches.

Make sure you know how patches are done before you send more. Wasting
maintainers' time with things which are already documented at large is not
nice.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette