Update ELF metadata #88

agrojean-ledger · 2023-10-06T11:53:27Z

Update infos.rs (add more metadatas to match C SDK)

Still missing :

APPNAME and APPVERSION : don't know how to fetch them at compile time of the SDK...
SDK_VERSION and SDK_NAME : fields are set to 'None' but we will give them a proper value when we transition to the new rust SDK with https://github.com/LedgerHQ/secure-sdk-rust

link.ld

agrojean-ledger · 2023-10-11T16:23:48Z

build.rs

-        // scott could use for_each
-        command.define(define, None);
-    }
+    extract_defines_from_cx_makefile(&mut command, &cx_makefile);


Added this for better readability but we can remove it.. Not related to what this PR is supposed to do.

Removed in #96

agrojean-ledger · 2023-10-12T11:13:37Z

@yhql I am missing APPNAME and APPVERSION, do you know how to add them when the rust SDK's build.rs is called ?

xchapron-ledger · 2023-10-12T13:08:15Z

src/infos.rs

+static ELF_API_LEVEL: [u8;4] = if cfg!(target_os = "nanos"){
+    *b"0\n\0\0"
+} else if cfg!(target_os = "nanox") {
+    *b"5\n\0\0"
+} else if cfg!(target_os = "nanosplus") {
+    *b"1\n\0\0"
+} else {
+    *b"255\n"
+};


Would need to be retrieved from the SDK, but this for after the change of architecture I guess?
Also, to be consistent with C sdk, the value for nanos should be : undefined, e.g. section doesn't exist.
and the else value might be the same ? Or a build error cause that's just invalid?

Yes in the new SDK we will grab it from the C SDK, it is already done actually here https://github.com/LedgerHQ/secure-sdk-rust/blob/f78cb5803b47de24ef7b2b0323b8b08ad31f7399/build.rs#L180

Done in #96

xchapron-ledger · 2023-10-18T15:51:26Z

src/infos.rs

+static ELF_TARGET: [u8; 8] = if cfg!(target_os = "nanos") {
+    *b"nanos\n\0\0"


Are there solutions to avoid these trailing "\n\0"? It seems a bit weird having them everywhere as I guess htey won't be used by the tools reading elf sections?

Hello,

We could use the following code:

let mut elf_target: [u8; 8] = [0u8; 8]; let _ = &mut elf_target[..5].copy_from_slice("nanos".as_bytes());

but cannot be called to define static const 😞 (and not sure it enhances readability)

So I propose to keep it as it is currently (until better proposal)

I'm wondering how these trailing \n and \0 will affect our tools reading metadata? Mostly Speculos and LedgerBlue for now.

Fixed in #96

yogh333 · 2023-10-19T08:23:02Z

src/lib.rs

@@ -5,6 +5,7 @@
 #![test_runner(testing::sdk_test_runner)]
 #![allow(incomplete_features)]
 #![feature(generic_const_exprs)]
+#![feature(const_cmp)]


Did not find this nightly feature in Unstable Book.

How did you find it ?

Where is it used ?

@yogh333 it's for let min_length = core::cmp::min(bytes.len(), MAX_METADATA_STRING_SIZE - 1);

But with @yhql 's proposed solution for strings, we probably would not need it.

And for how I found it, I can't remember ^^

We can remove this if we replace this min by a more manual version:

let min_length = if bytes.len() <= MAX_METADATA_STRING_SIZE - 1 { bytes.len() } else { MAX_METADATA_STRING_SIZE - 1 };

Removed in #96

xchapron-ledger · 2023-11-17T13:10:35Z

src/infos.rs

+#[used]
+#[link_section = "ledger.api_level"]
+static ELF_API_LEVEL: [u8; 4] = if cfg!(target_os = "nanos") {
+    *b"0\n\0\0"
+} else if cfg!(target_os = "nanox") {
+    *b"5\n\0\0"
+} else if cfg!(target_os = "nanosplus") {
+    *b"1\n\0\0"
+} else {
+    *b"255\n"
+};


Should be update now that API_LEVEL is now at build time from SDK?

Done in #96

xchapron-ledger · 2023-11-17T13:11:02Z

src/infos.rs

+    result
+}
+
+/// Expose the API_LEVEL to the app
 #[used]
 static API_LEVEL: u8 = if cfg!(target_os = "nanos") {


Should be update to with proper value knwon at build time ?

Done in #96

xchapron-ledger · 2023-11-17T13:13:12Z

src/infos.rs

+
+#[used]
+#[link_section = "ledger.sdk_hash"]
+static ELF_SDK_HASH: [u8; 5] = *b"None\n";


I think we can live with this, but best would be having the hash of the C SDK used.

Done in #96

xchapron-ledger · 2023-11-17T13:13:55Z

src/infos.rs

+
+#[used]
+#[link_section = "ledger.sdk_name"]
+static ELF_SDK_NAME: [u8; 18] = *b"ledger-secure-sdk\n";


in case of a nano, with should probably set nanos-secure-sdk as it doesn't use the unified one?

Done in #96

xchapron-ledger · 2023-11-17T13:14:23Z

src/infos.rs

+
+#[used]
+#[link_section = "ledger.sdk_version"]
+static ELF_SDK_VERSION: [u8; 5] = *b"None\n";


Same as for the hash, would be best to have the real C SDK version, else that is not a drama.

Done in #96

xchapron-ledger

I think some stuff could be up^date now that we use ledger_secure_sdk_sys crate.
We could use the env declared here: https://github.com/LedgerHQ/ledger-device-rust-sdk/blob/master/ledger_secure_sdk_sys/build.rs#L208-L210
And maybe add some more to populate properly SDK_HASH / SDK_NAME and SDK_VERSION ?

agrojean-ledger · 2023-11-22T13:28:13Z

Replaced by #96

agrojean-ledger force-pushed the update-app-metadata branch 2 times, most recently from 483b180 to 8697436 Compare October 11, 2023 16:20

Add additional metadata in infos.rs

bf3db8b

agrojean-ledger force-pushed the update-app-metadata branch from 8697436 to bf3db8b Compare October 11, 2023 16:21

agrojean-ledger commented Oct 11, 2023

View reviewed changes

link.ld Show resolved Hide resolved

agrojean-ledger commented Oct 11, 2023

View reviewed changes

agrojean-ledger requested a review from yhql October 11, 2023 16:24

xchapron-ledger reviewed Oct 12, 2023

View reviewed changes

agrojean-ledger added 2 commits October 13, 2023 10:36

Update SDK version in Cargo.toml

6d67b5a

Format

7520611

agrojean-ledger requested a review from yogh333 October 18, 2023 12:04

xchapron-ledger reviewed Oct 18, 2023

View reviewed changes

yogh333 reviewed Oct 19, 2023

View reviewed changes

Merge branch 'master' into update-app-metadata

624fbff

yogh333 closed this Nov 14, 2023

yogh333 deleted the update-app-metadata branch November 14, 2023 08:18

yogh333 restored the update-app-metadata branch November 16, 2023 21:52

yogh333 reopened this Nov 16, 2023

xchapron-ledger reviewed Nov 17, 2023

View reviewed changes

agrojean-ledger closed this Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ELF metadata #88

Update ELF metadata #88

agrojean-ledger commented Oct 6, 2023 •

edited

Loading

agrojean-ledger Oct 11, 2023

agrojean-ledger Nov 22, 2023

agrojean-ledger commented Oct 12, 2023 •

edited

Loading

xchapron-ledger Oct 12, 2023

agrojean-ledger Oct 12, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Oct 18, 2023

yogh333 Oct 19, 2023

yogh333 Oct 19, 2023

xchapron-ledger Nov 22, 2023

agrojean-ledger Nov 22, 2023

yogh333 Oct 19, 2023 •

edited

Loading

agrojean-ledger Oct 19, 2023

agrojean-ledger Oct 19, 2023

agrojean-ledger Oct 19, 2023

yhql Oct 19, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Nov 17, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Nov 17, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Nov 17, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Nov 17, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger Nov 17, 2023

agrojean-ledger Nov 22, 2023

xchapron-ledger left a comment

agrojean-ledger commented Nov 22, 2023

		static ELF_TARGET: [u8; 8] = if cfg!(target_os = "nanos") {
		*b"nanos\n\0\0"

Update ELF metadata #88

Update ELF metadata #88

Conversation

agrojean-ledger commented Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrojean-ledger commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogh333 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xchapron-ledger left a comment

Choose a reason for hiding this comment

agrojean-ledger commented Nov 22, 2023

agrojean-ledger commented Oct 6, 2023 •

edited

Loading

agrojean-ledger commented Oct 12, 2023 •

edited

Loading

yogh333 Oct 19, 2023 •

edited

Loading