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

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

[Shresth Prasad]

Prevent a NULL pointer dereference in snp_kexec_finish() by checking the
value returned by lookup_address() call.

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

Fixes: 3074152e56c9 ("x86/sev: Convert shared memory back to private on kexec")
Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
---
Changes in v2:
- Reword commit message
- Add Fixes tag
- Link to v1: https://lore.kernel.org/r/20241119-fix-dereference-null-x86-sev-v1-1-82d59085e264@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 v2] x86/sev: Fix dereference NULL return value

[Borislav Petkov]

On Wed, Nov 20, 2024 at 02:21:13AM +0530, Shresth Prasad wrote:
> Prevent a NULL pointer dereference in snp_kexec_finish() by checking the
> value returned by lookup_address() call.

Can this really happen?

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

I can't open this page - all coverity folks: you either describe what the
issue is or don't bother sending patches.

> Fixes: 3074152e56c9 ("x86/sev: Convert shared memory back to private on kexec")

So I'd hope if you report a bug against some patch, the least you could do is
CC its author...

To give you the same spiel:

Please 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.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Shresth Prasad]

On Wed, Nov 20, 2024 at 2:33 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Nov 20, 2024 at 02:21:13AM +0530, Shresth Prasad wrote:
> > Prevent a NULL pointer dereference in snp_kexec_finish() by checking the
> > value returned by lookup_address() call.
>
> Can this really happen?

lookup_address() does return NULL in some paths so I do assume that it
can happen, unless there's a logical reason why it can't (please let me know if
that's the case). I've also seen it be checked this way in a couple
other places.

>
> > This issue was reported by Coverity scan:
> > https://scan7.scan.coverity.com/#/project-view/52279/11354?selectedIssue=1601527
>
> I can't open this page - all coverity folks: you either describe what the
> issue is or don't bother sending patches.

I'm not sure why you can't open the page but would it help if I was more
descriptive in the commit message?

>
> > Fixes: 3074152e56c9 ("x86/sev: Convert shared memory back to private on kexec")
>
> So I'd hope if you report a bug against some patch, the least you could do is
> CC its author...

Really sorry about that, I completely overlooked it. I'll CC them
when I resend the patch.

Best Regards,
Shresth

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

[Borislav Petkov]

On Wed, Nov 20, 2024 at 06:23:09PM +0530, Shresth Prasad wrote:
> lookup_address() does return NULL in some paths so I do assume that it

You assume?

Well, do you know or not? You can simply read lookup_address() and more
specifically lookup_address_in_pgd_attr() and see whether it can return NULL
or not.

As to this particular case, I don't think it would return a NULL. Otherwise
something else is very very wrong so perhaps it is better to crash'n'burn
there.

What would happen if you continue instead on a NULL ptr? Would that make sense
either?

Basically, I'm trying to make you think before you send patches. Just because
some silly tool says something is wrong, it doesn't mean you should trust it
blindly.

You analyze the situation and *then* you send a patch, only when it is really
an issue.

> can happen, unless there's a logical reason why it can't (please let me know
> if that's the case). I've also seen it be checked this way in a couple other
> places.

Kernel programming is not voodoo. You read the code and think.

> I'm not sure why you can't open the page but would it help if I was more
> descriptive in the commit message?

SYNOPSYS

Username:
Password:

is what I get.

> Really sorry about that, I completely overlooked it. I'll CC them
> when I resend the patch.

Before you do, I'd like you to turn on brain and think about the questions
above.

And I'd like you to please 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.

I'm not typing those to get ignored. I mean, I can ignore emails too if mine
get ignored.

-- 
Regards/Gruss,
    Boris.

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

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

[Shresth Prasad]

On Thu, Nov 21, 2024 at 12:33 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Nov 20, 2024 at 06:23:09PM +0530, Shresth Prasad wrote:
> > lookup_address() does return NULL in some paths so I do assume that it
>
> You assume?
>
> Well, do you know or not? You can simply read lookup_address() and more
> specifically lookup_address_in_pgd_attr() and see whether it can return NULL
> or not.
>
> As to this particular case, I don't think it would return a NULL. Otherwise
> something else is very very wrong so perhaps it is better to crash'n'burn
> there.
>
> What would happen if you continue instead on a NULL ptr? Would that make sense
> either?
>
> Basically, I'm trying to make you think before you send patches. Just because
> some silly tool says something is wrong, it doesn't mean you should trust it
> blindly.
>
> You analyze the situation and *then* you send a patch, only when it is really
> an issue.
>
> > can happen, unless there's a logical reason why it can't (please let me know
> > if that's the case). I've also seen it be checked this way in a couple other
> > places.
>
> Kernel programming is not voodoo. You read the code and think.
>
> > I'm not sure why you can't open the page but would it help if I was more
> > descriptive in the commit message?
>
> SYNOPSYS
>
> Username:
> Password:
>
> is what I get.
>
> > Really sorry about that, I completely overlooked it. I'll CC them
> > when I resend the patch.
>
> Before you do, I'd like you to turn on brain and think about the questions
> above.
>
> And I'd like you to please 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.
>
> I'm not typing those to get ignored. I mean, I can ignore emails too if mine
> get ignored.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

I apologise, my approach here was incorrect. I'll rethink how I submit
patches from now on.
It wasn't my intention to ignore any part of your message. I
absolutely went through the docs that you linked.

Thank you for your time.

Best Regards,
Shresth