rust-embedded/cortex-m

Incorrect implementation of `cortex_m::peripheral::scb::VectActive::from()` in stable release 0.7.7

skibon02 opened this issue · 7 comments

skibon02 commented

I discovered this bug accidentally while working with SCB and exceptions. cortex_m::peripheral::scb::VectActive::from() function returns incorrect value, which is 16 higher than the expected value. (in case when VectActive is VectActive::Interrupt.
I know this was already fixed in #332, but it was really long time ago (2 years ago) and this change is not present in the current released version uploaded on crates.io.

https://github.com/rust-embedded/cortex-m/blob/bb4a78208323260a161e68b2498438867f971bc5/src/peripheral/scb.rs#L303C15-L303C15

Since the fix is already implemented in the master branch, I am curious about the timeline for the next release on crates.io. Alternatively, is it feasible to release a patch for version 0.7.7 specifically to address this bug?

skibon02 commented

We can look by example, 303 line is

cortex-m/src/peripheral/scb.rs

Line 303 in bb4a782

irqn if irqn >= 16 => VectActive::Interrupt { irqn },

So, it returns Interrupt with value irqn, while irqn is greater or equal to 16.

In contrast, we have another function SCB::vect_active() which does the same (converting number to VectActive) and therefore should have the same implementation, but in fact it differs:

cortex-m/src/peripheral/scb.rs

Line 192 in bb4a782

irqn => VectActive::Interrupt { irqn: irqn - 16 },

jonathanpallant commented

Ugh, good spot.

Why do we have the same code pasted twice? fn vect_active() can just call .into(), surely?

skibon02 commented

I guess so, but the only difference is that we'll need .unwrap() the Option, and it's better to keep guard if irqn >= 16 just in case

adamgreig commented

Thanks for reporting this! I think we should backport the - 16 fix to 0.7.x and release 0.7.8 with it, but keeping u8 for now as it would be a breaking change otherwise. Would you like to open a PR (against the v0.7.x branch)? If there's a nice way to combine the vect_active implementation too then go for it, but i agree it's nice to avoid the unwrap and keep the guard.

skibon02 commented

@adamgreig, Sure, I can do it.

But how can we avoid using unwrap when we must cover all possible values in match? I guess it's ok to believe the register value we got from reading current vector register and assume correct arm target selected by user.
Panic on incorrect vector value would be a better approach than undefined behavior we can get in the current implementation of vect_active function (for example if we got secure fault on target other than armv8, we got irqn - 16 = 7 - 16 => overflow in release build). Not totally sure such case is possible but seems like a good approach for safety to panic on unknown value.