[PATCH] x86/sev: Fix dereference NULL return value

Shresth Prasad posted 1 patch 4 days, 6 hours ago
There is a newer version of this series
arch/x86/coco/sev/core.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] x86/sev: Fix dereference NULL return value
Posted by Shresth Prasad 4 days, 6 hours ago
Skip to the next CPU if `pte` is assigned NULL by `lookup_address`

This issue was reported by Coverity scan:
https://scan7.scan.coverity.com/#/project-view/52279/11354?selectedIssue=1601527

Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
---
 arch/x86/coco/sev/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index c5b0148b8c0a191f5aa38af73a52c77d6bba3e2d..0436366243e19a72bf9521f2e96a3ceec9c1270c 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1079,6 +1079,8 @@ void snp_kexec_finish(void)
 		data = per_cpu(runtime_data, cpu);
 		ghcb = &data->ghcb_page;
 		pte = lookup_address((unsigned long)ghcb, &level);
+		if (!pte)
+			continue;
 		size = page_level_size(level);
 		set_pte_enc(pte, level, (void *)ghcb);
 		snp_set_memory_private((unsigned long)ghcb, (size / PAGE_SIZE));

---
base-commit: 28955f4fa2823e39f1ecfb3a37a364563527afbc
change-id: 20241119-fix-dereference-null-x86-sev-4b62d89e8b98

Best regards,
-- 
Shresth Prasad <shresthprasad7@gmail.com>
Re: [PATCH] x86/sev: Fix dereference NULL return value
Posted by Markus Elfring 3 days, 23 hours ago
> Skip to the next CPU if `pte` is assigned NULL by `lookup_address`

                          a null pointer was returned by a lookup_address() call.


* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145

* Would a summary phrase like “Prevent null pointer dereference in snp_kexec_finish()”
  be a bit nicer?


Regards,
Markus
Re: [PATCH] x86/sev: Fix dereference NULL return value
Posted by Shresth Prasad 3 days, 19 hours ago
On Tue, Nov 19, 2024 at 5:33 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Skip to the next CPU if `pte` is assigned NULL by `lookup_address`
>
>                           a null pointer was returned by a lookup_address() call.
>
>
> * How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

For the Fixes tag, I'm not sure which commit hash I should put. Should I use
the commit where the function was introduced?

>
> * Would a summary phrase like “Prevent null pointer dereference in snp_kexec_finish()”
>   be a bit nicer?

You're right, I'll change the phrasing. Thank you for the feedback.

Best Regards,
Shresth
Re: x86/sev: Fix dereference NULL return value
Posted by Markus Elfring 3 days, 18 hours ago
> For the Fixes tag, I'm not sure which commit hash I should put. Should I use
> the commit where the function was introduced?

How far can the overlooked function return value be traced back?

Regards,
Markus
Re: x86/sev: Fix dereference NULL return value
Posted by Shresth Prasad 3 days, 17 hours ago
On Tue, Nov 19, 2024 at 11:03 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > For the Fixes tag, I'm not sure which commit hash I should put. Should I use
> > the commit where the function was introduced?
>
> How far can the overlooked function return value be traced back?

Using git blame I see that snp_kexec_finish() was first introduced in
3074152e56c9b
and has stayed that way since. Should I just use that hash?

Best Regards,
Shresth
Re: x86/sev: Fix dereference NULL return value
Posted by Markus Elfring 3 days, 16 hours ago
> Using git blame I see that snp_kexec_finish() was first introduced in
> 3074152e56c9b
> and has stayed that way since. Should I just use that hash?
Probably, yes.
Would you insist to use only 12 characters from such an identifier?

Regards,
Markus
Re: x86/sev: Fix dereference NULL return value
Posted by Shresth Prasad 3 days, 15 hours ago
On Wed, Nov 20, 2024 at 12:26 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Using git blame I see that snp_kexec_finish() was first introduced in
> > 3074152e56c9b
> > and has stayed that way since. Should I just use that hash?
> Probably, yes.
> Would you insist to use only 12 characters from such an identifier?

I'm not sure what you mean but as suggested by the docs I'll use the first 12
characters. Thank you for your help, I'll make these changes and send a v2.

Best Regards,
Shresth