Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Efuse::read_field_{le, be} can cause Undefined Behavior. #2228

Closed
27factorial opened this issue Sep 25, 2024 · 2 comments · Fixed by #2259
Closed

Efuse::read_field_{le, be} can cause Undefined Behavior. #2228

27factorial opened this issue Sep 25, 2024 · 2 comments · Fixed by #2259
Assignees
Labels
bug Something isn't working
Milestone

Comments

@27factorial
Copy link

27factorial commented Sep 25, 2024

Bug description

Efuse::read_field_le and Efuse::read_field_be put no restrictions on the types that they can return, other than the Sized + 'static constraint. This means that types which require certain invariants to be upheld (such as references) can be constructed in ways which break those invariants and cause UB.

To Reproduce

  1. Run cargo generate esp-rs/esp-template and create a new ESP32-S3 Rust binary.
  2. Copy the example below into src/main.rs
  3. Execute cargo run --release

Example

#![no_std]
#![no_main]

use esp_hal::efuse::{Efuse, ADC1_CAL_VOL_ATTEN0};
use esp_println::dbg;

#[esp_hal::entry]
fn main() -> ! {
    let example: &'static usize = Efuse::read_field_le(ADC1_CAL_VOL_ATTEN0);

    dbg!(example);

    loop {
        continue;
    }
}

This example also works on the main branch, as well.

Output

[src/main.rs:134] example = 


Exception occured 'LoadProhibited'
Context
PC=0x42000fb3       PS=0x00060930
0x42000fb3 - <u32 as core::fmt::num::DisplayInt>::to_u32
    at /home/username/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/fmt/num.rs:44
A0=0x82001091       A1=0x3fcdc580       A2=0x00000079       A3=0x3fcdc5fc       A4=0x00000000
A5=0xffff0000       A6=0x42000000       A7=0x3c010000       A8=0x3fceb340       A9=0x3fcdc700
A10=0x00000002      A11=0x3fce380c      A12=0x4200462f      A13=0x00000003      A14=0x00000001
A15=0x00000000
SAR=00000018
EXCCAUSE=0x0000001c EXCVADDR=0x00000079
LBEG=0x40056f5c     LEND=0x40056f72     LCOUNT=0xffffffff
THREADPTR=0x00000000
SCOMPARE1=0x00000100
BR=0x00000000
ACCLO=0x00000000    ACCHI=0x00000000
M0=0x00000000       M1=0x00000000       M2=0x00000000       M3=0x00000000
F64R_LO=0x00000000  F64R_HI=0x00000000  F64S=0x00000000
FCR=0x00000000      FSR=0x00000000
F0=0x00000000       F1=0x00000000       F2=0x00000000       F3=0x00000000       F4=0x00000000
F5=0x00000000       F6=0x00000000       F7=0x00000000       F8=0x00000000       F9=0x00000000
F10=0x00000000      F11=0x00000000      F12=0x00000000      F13=0x00000000      F14=0x00000000
F15=0x00000000
0x420007ab
<&T as core::fmt::Debug>::fmt
    at /home/username/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2368
0x42000ea4
<core::iter::adapters::enumerate::Enumerate<I> as core::iter::traits::iterator::Iterator>::next
    at /home/username/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/iter/adapters/enumerate.rs:50
0x42003f6f
<&mut W as core::fmt::Write::write_fmt::SpecWriteFmt>::spec_write_fmt
    at /home/username/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:230
0x4200078b
example::__xtensa_lx_rt_main
    at /home/username/Development/ESP32/example/src/main.rs:136
0x42004249
Reset
    at ??:??
0x40378725
ESP32Reset
    at ??:??
0x40000000
0x403cdd7c
0x403c9974
0x40045c04

Expected behavior

Using Efuse::read_field_{le, be} to construct arbitrary types should be prevented to not cause UB.

Environment

  • Target device:
Chip type:         esp32s3 (revision v0.2)
Crystal frequency: 40 MHz
Flash size:        8MB
Features:          WiFi, BLE
  • Crate name and version: esp-hal 0.20.1
@27factorial 27factorial added bug Something isn't working status:needs-attention This should be prioritized labels Sep 25, 2024
@MabezDev
Copy link
Member

Thanks for the report!

This is definitely an issue, I see two ways forward:

  • We mark this as unsafe and make it the responsibility of the caller
  • We add an addition trait bound using something like bytemuck::AnyBitPattern to ensure that UB cannot occur

My preferred option would be the latter, but I haven't fully explored whether this covers all cases.

@MabezDev MabezDev removed the status:needs-attention This should be prioritized label Sep 25, 2024
@27factorial
Copy link
Author

Sorry for the late response.

I'm pretty certain that something along the lines of AnyBitPattern would be sufficient. The safety requirements for implementing that trait rule out things like references and bools, while also letting esp_hal continue to use those methods as it currently does, since all uses are with types that implement AnyBitPattern.

@MabezDev MabezDev modified the milestones: 0.22.0, 0.21.0 Sep 30, 2024
@bugadani bugadani self-assigned this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants