embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.32k stars 734 forks source link

wrong reading stride on `uid::uid_hex()` #3046

Open shufps opened 3 months ago

shufps commented 3 months ago

Hi,

this is a bit strange because the UID should be unique but I always get the same ID.

I'm using the UID for the serial number of my USB and I got 333347053438333411000000 on multiple STM32L072 :thinking:

I tried it like this:

    // Create embassy-usb Config
    let mut config = embassy_usb::Config::new(0xc0de, 0xcafe);
    config.max_packet_size_0 = 64;
    config.manufacturer = Some("xxx");
    config.product = Some("yyy");
    config.serial_number = Some(uid::uid_hex());

I can see then a device:

/dev/serial/by-id/usb-xxx_yyy_333347053438333411000000-if000

but with different boards it's always the same.

What am I doing wrong? :see_no_evil:

shufps commented 3 months ago

hmm, the memory address of the UID is correct in the PAC for the L072 :thinking:

shufps commented 3 months ago

This works:

fn read_uid() {
    let base_address: usize = 0x1FF80050;

    let mut unique_id = [0u32;3];
    unsafe {
        unique_id = [
            ((base_address) as *const u32).read_volatile(),
            ((base_address+0x4) as *const u32).read_volatile(),
            ((base_address+0x14) as *const u32).read_volatile(),
        ];
    }
    info!("Unique ID: {:08X} {:08X} {:08X}", unique_id[0], unique_id[1], unique_id[2]);
}

probably there is an embassy bug because it probably assumes that all three 32bit words would follow directly after each other.

There even seems to be other inconsistencies like some STM32 having the 3rd word at offset 0x12. The L072 has it on 0x14 :see_no_evil:

shufps commented 3 months ago

This could be security relevant if UID is used as seed to initialize crypto things :thinking:

Dirbaio commented 3 months ago

If you want this fixed, please send a PR instead of invoking the "could be security relevant" card.

shufps commented 3 months ago

If you want this fixed, please send a PR instead of invoking the "could be security relevant" card.

Ok, playing the "told you so" card somewhen in the future :rofl: Just kidding.

Tbh, I can't fix it because it's above my skill level. I contributed a couple of other things but I guess it would require deeper changes how the PAC is defined because it just has a base address and a "stride". But it's not prepared for reading from offsets like 0x00, 0x04 and 0x14.

So idk, can't do anything here :man_shrugging:

Dirbaio commented 3 months ago

why close it? it's not fixed