core: check guest_id param passed when recieving SMC from NW #7189

yuvraj1803 · 2024-12-18T04:51:18Z

Certain SMCs such as OPTEE_SMC_VM_CREATED can send OPTEE to panic if guest ID passed is 0.

P.S. sorry for the second PR, there was something really wrong with my branch.

etienne-lms

For commit "core: arm: add guest id check at VM creation": maybe start the commit message with the last sentence, e.g. rephrased here:

Forbid creation of non-secure world guests using the hypervisor ID (0)
that is reserved.

Normal world calls ...

For commit "core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF":
commit message could be shorten IMHO and deserves a Fixes: tag;,e.g.:

Set guest ID in NOTIF_EVENT_STARTED notification that was missing
causing a panic (segfault or assertion error) in get_notif_data().

Fixes: d237e616e155 ("core: make generic notifications virtualization-aware")
S-o-b: ...

core/arch/arm/tee/entry_fast.c

core/arch/arm/kernel/virtualization.c

jenswi-linaro · 2024-12-19T06:31:51Z

Here's an attempt at taking care of the assert in notif_send_async(), https://github.com/jenswi-linaro/optee_os/tree/wip/virt_notif
I haven't been able to test it yet, but it compiles. So what could go wrong? :-)
Please add it to this PR if it helps solve your problem.

yuvraj1803 · 2024-12-19T07:20:33Z

Here's an attempt at taking care of the assert in notif_send_async(), https://github.com/jenswi-linaro/optee_os/tree/wip/virt_notif I haven't been able to test it yet, but it compiles. So what could go wrong? :-) Please add it to this PR if it helps solve your problem.

Thanks @jenswi-linaro. Tested. Works like a charm.

jenswi-linaro

Please squash in the updates on the first two commits. You'll need to do a git push -f to update the pull request.

jenswi-linaro · 2024-12-19T09:41:50Z

core/kernel/notif_default.c

 	uint32_t old_itr_status = 0;
 	struct itr_chip *itr_chip = interrupt_get_main_chip();

-	assert(value <= NOTIF_ASYNC_VALUE_MAX && !guest_id);
+


Please remove this duplicated blank line that checkpatch complains over, I must have added it by mistake.

etienne-lms

Could you squash the fixup in commit "core: arm: guest ID test for notif and VM creation" info each of the related 2 previous commits of your series?

Acked-by: Etienne Carriere <[email protected]> for the series with the minor comments in "core: notif_default: support ns-virtualization" addressed.

core/kernel/notif_default.c

jenswi-linaro · 2024-12-20T07:01:03Z

Please fix the checkpatch issues. The bit_decl() macro is known to confuse checkpatch so those warnings can be ignored.

jenswi-linaro · 2024-12-20T07:23:11Z

You can run checkpatch on the patches in your tree with:

./scripts/checkpatch.sh github/master..

assuming that the name of the remote is github and your patches are on top of the branch master.

yuvraj1803 · 2024-12-20T07:39:58Z

You can run checkpatch on the patches in your tree with:
./scripts/checkpatch.sh github/master..
assuming that the name of the remote is github and your patches are on top of the branch master.

Sorry, overlooked the second commit :)

jenswi-linaro · 2024-12-20T07:51:14Z

These should be fixed:

ac62c5965 core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF
WARNING: braces {} are not necessary for any arm of this statement
#28: FILE: core/arch/arm/tee/entry_fast.c:294:
+                       if (IS_ENABLED(CFG_NS_VIRTUALIZATION)) {
[...]
+                       } else {
[...]

WARNING: line length of 90 exceeds 80 columns
#29: FILE: core/arch/arm/tee/entry_fast.c:295:
+                               notif_deliver_atomic_event(NOTIF_EVENT_STARTED, args->a7);

WARNING: line length of 83 exceeds 80 columns
#31: FILE: core/arch/arm/tee/entry_fast.c:297:
+                               notif_deliver_atomic_event(NOTIF_EVENT_STARTED, 0);

WARNING: From:/Signed-off-by: email name mismatch: 'From: yuvraj1803 <[email protected]>' != 'Signed-off-by: Yuvraj Sakshith <[email protected]>'

total: 0 errors, 4 warnings, 0 checks, 12 lines checked
>> checkpatch f365f68ec1b65349fe2a04679f32e31325c4f575
f365f68ec core: arm: add guest id check at VM creation
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#6:
Normal world calls OPTEE_SMC_VM_CREATED with guest VMID in a1 and HYP_CLNT_ID

WARNING: From:/Signed-off-by: email name mismatch: 'From: yuvraj1803 <[email protected]>' != 'Signed-off-by: Yuvraj Sakshith <[email protected]>'

total: 0 errors, 2 warnings, 0 checks, 9 lines checked

yuvraj1803 · 2024-12-20T08:17:11Z

Noted.

etienne-lms

@yuvraj1803 , could you discard commit "core: arm: add guest id check at VM creation"
and have all changes related to virt_guest_created() into commit "core: arm: guest ID test for notif and VM creation".

etienne-lms · 2024-12-20T10:45:46Z

core/kernel/notif_default.c

-static bitstr_t bit_decl(notif_alloc_values, NOTIF_ASYNC_VALUE_MAX + 1);
+struct notif_vm_bitmap {
+	bool alloc_values_inited;
+


could remove this empty line.

Sure. There was a warning in checkpatch that was complaining about there not being an empty line.

checkdiff 383d059 37e5be4
WARNING: Missing a blank line after declarations
#60: FILE: core/kernel/notif_default.c:19:

bool alloc_values_inited;

bitstr_t bit_decl(values, NOTIF_ASYNC_VALUE_MAX + 1);

yuvraj1803 · 2024-12-20T12:32:16Z

@etienne-lms done.

etienne-lms

For commit "core: notif_default: support ns-virtualization":
@yuvraj1803 I think you should add your Signed-off-of tag for this patch, next to Jens' one, since it's part of your P-R (see checkpatch complaint about author's S-o-b tag absence).
With that addressed: Reviewed-by: Etienne Carriere <[email protected]> for the 3 commits.

The other checkpatch warnings around bit_decl() are false positive.

jenswi-linaro · 2024-12-20T14:27:24Z

For commit "core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF", please remove
this from the commit message:

Forcing the default partition into the MMU by core_mmu_set_default_prtn() does not seem to work as there seem to be different page tables for each CPU. This can place the CPU into panic after check_pa_matches_va() makes an assertion check.

With that fixed please apply:
Reviewed-by: Jens Wiklander <[email protected]>
for "core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF" and "core: arm: guest ID test for notif and VM creation

yuvraj1803 · 2024-12-20T15:03:13Z

For commit "core: notif_default: support ns-virtualization": @yuvraj1803 I think you should add your Signed-off-of tag for this patch, next to Jens' one, since it's part of your P-R (see checkpatch complaint about author's S-o-b tag absence). With that addressed: Reviewed-by: Etienne Carriere <[email protected]> for the 3 commits.

The other checkpatch warnings around bit_decl() are false positive.

Noted. Thanks @etienne-lms

yuvraj1803 · 2024-12-20T15:03:36Z

For commit "core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF", please remove this from the commit message:

Forcing the default partition into the MMU by core_mmu_set_default_prtn() does not seem to work as there seem to be different page tables for each CPU. This can place the CPU into panic after check_pa_matches_va() makes an assertion check.

With that fixed please apply: Reviewed-by: Jens Wiklander <[email protected]> for "core: arm: pass guest ID to SMC_ENABLE_ASYNC_NOTIF" and "core: arm: guest ID test for notif and VM creation

Changes made, @jenswi-linaro.

etienne-lms · 2025-01-07T17:41:11Z

Reviewed-by: Etienne Carriere <[email protected]> for the 3 commits.
Sorry for this late feedback.

Maybe rebasing the series will address the 'failing CI / make check' issue.

yuvraj1803 · 2025-01-08T12:55:31Z

Reviewed-by: Etienne Carriere <[email protected]> for the 3 commits. Sorry for this late feedback.

Maybe rebasing the series will address the 'failing CI / make check' issue.

Thanks @etienne-lms

jforissier · 2025-01-08T13:12:27Z

@yuvraj1803 please add @etienne-lms' Reviewed-by: tags. Thanks!

notif_deliver_atomic_event() expects guest_id which is used to retrieve struct guest_partition* from virt_get_guest(). The guest_id passed is static (0), which causes trouble when OPTEE_SMC_ENABLE_ASYNC_NOTIF comes from a guest. When this happens, virt_get_guest() returns NULL which fails the assertion in get_notif_data() which exclusively checks for CONFIG_NS_VIRTUALIZATION. Signed-off-by: Yuvraj Sakshith <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Block normal world from calling OPTEE_SMC_VM_CREATED with reserved hypervisor client-id (0) as VMID parameter. Normal world calls OPTEE_SMC_VM_CREATED with guest VMID in a1 and HYP_CLNT_ID in a7. This eventually leads to copying of __data_start to __data_end from the default partition to the guest's MMU partition. Everything goes well until normal world passes HYP_CLNT_ID into a1 which goes unchecked in OPTEE. When the "second VM" is created from normal world, the first VM's MMU partition's __data_start is copied into the new VM's MMU partition which eventually breaks the bpool freelist pointers. This can deliberately be used by normal world to put OP-TEE into panic. Set guest ID when NOTIF_EVENT_STARTED is called preventing assetion failure in get_notif_data(). Fixes: d237e61 ("core: make generic notifications virtualization-aware") Signed-off-by: Yuvraj Sakshith <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Add support for CFG_NS_VIRTUALIZATION=y in the default notification implementation used with the SMC ABI. virt_add_guest_spec_data() is used to add struct notif_vm_bitmap for bookkeeping per guest, similarly to the implementation for the FF-A ABI. This takes care of and removes the assert for "!guest" in notif_send_async(). Signed-off-by: Jens Wiklander <[email protected]> Signed-off-by: Yuvraj Sakshith <[email protected]> Tested-by: Yuvraj Sakshith <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

yuvraj1803 · 2025-01-08T13:18:54Z

@yuvraj1803 please add @etienne-lms' Reviewed-by: tags. Thanks!

Checked. Thanks! @jforissier

yuvraj1803 · 2025-01-08T14:49:50Z

This was my first ever contribution.

Thanks for your patience and support :) @jenswi-linaro @etienne-lms @jforissier

etienne-lms reviewed Dec 18, 2024

View reviewed changes

core/arch/arm/tee/entry_fast.c Outdated Show resolved Hide resolved

core/arch/arm/kernel/virtualization.c Outdated Show resolved Hide resolved

jenswi-linaro reviewed Dec 19, 2024

View reviewed changes

yuvraj1803 force-pushed the vt_gid_check branch from 44f1f68 to 944f1ed Compare December 19, 2024 10:38

etienne-lms reviewed Dec 19, 2024

View reviewed changes

core/kernel/notif_default.c Show resolved Hide resolved

yuvraj1803 force-pushed the vt_gid_check branch 2 times, most recently from ae0530f to dbbdf7a Compare December 19, 2024 16:31

yuvraj1803 force-pushed the vt_gid_check branch from dbbdf7a to 8088154 Compare December 20, 2024 07:16

yuvraj1803 force-pushed the vt_gid_check branch from 8088154 to c54cc2a Compare December 20, 2024 07:39

yuvraj1803 force-pushed the vt_gid_check branch from c54cc2a to a02c40b Compare December 20, 2024 08:16

etienne-lms reviewed Dec 20, 2024

View reviewed changes

yuvraj1803 force-pushed the vt_gid_check branch from a02c40b to 37e5be4 Compare December 20, 2024 12:31

etienne-lms reviewed Dec 20, 2024

View reviewed changes

yuvraj1803 force-pushed the vt_gid_check branch from 37e5be4 to dd127cb Compare December 20, 2024 15:01

yuvraj1803 force-pushed the vt_gid_check branch from dd127cb to 565396f Compare January 8, 2025 12:54

yuvraj1803 added 2 commits January 8, 2025 18:46

yuvraj1803 force-pushed the vt_gid_check branch from 565396f to c4223af Compare January 8, 2025 13:17

jforissier merged commit 09d7477 into OP-TEE:master Jan 8, 2025
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: check guest_id param passed when recieving SMC from NW #7189

core: check guest_id param passed when recieving SMC from NW #7189

yuvraj1803 commented Dec 18, 2024

etienne-lms left a comment

jenswi-linaro commented Dec 19, 2024

yuvraj1803 commented Dec 19, 2024 •

edited

Loading

jenswi-linaro left a comment

jenswi-linaro Dec 19, 2024

yuvraj1803 Dec 19, 2024

etienne-lms left a comment

jenswi-linaro commented Dec 20, 2024

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

etienne-lms left a comment

etienne-lms Dec 20, 2024

yuvraj1803 Dec 20, 2024

yuvraj1803 Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

etienne-lms left a comment

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024 •

edited

Loading

etienne-lms commented Jan 7, 2025

yuvraj1803 commented Jan 8, 2025

jforissier commented Jan 8, 2025

yuvraj1803 commented Jan 8, 2025

yuvraj1803 commented Jan 8, 2025

core: check guest_id param passed when recieving SMC from NW #7189

core: check guest_id param passed when recieving SMC from NW #7189

Conversation

yuvraj1803 commented Dec 18, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Dec 19, 2024

yuvraj1803 commented Dec 19, 2024 • edited Loading

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro Dec 19, 2024

Choose a reason for hiding this comment

yuvraj1803 Dec 19, 2024

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Dec 20, 2024

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms Dec 20, 2024

Choose a reason for hiding this comment

yuvraj1803 Dec 20, 2024

Choose a reason for hiding this comment

yuvraj1803 Dec 20, 2024

Choose a reason for hiding this comment

yuvraj1803 commented Dec 20, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024

yuvraj1803 commented Dec 20, 2024 • edited Loading

etienne-lms commented Jan 7, 2025

yuvraj1803 commented Jan 8, 2025

jforissier commented Jan 8, 2025

yuvraj1803 commented Jan 8, 2025

yuvraj1803 commented Jan 8, 2025

yuvraj1803 commented Dec 19, 2024 •

edited

Loading

yuvraj1803 commented Dec 20, 2024 •

edited

Loading