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

Compile secp256 in U-boot #603

Closed
wants to merge 1 commit into from
Closed

Conversation

tmagik
Copy link

@tmagik tmagik commented Mar 26, 2019

This pull request is more for discussion, and get some commentary on the value of having device firmware (such as u-boot) that can verify DSA signatures of kernel and userspace payloads.

Things that I need are some help with the simplest example code to check DSA signatures, and any pointers to any other smaller embedded DSA signature codes, or some discussion on whether adapting secp256k to this application is even a good idea.

In particular, maybe @laanwj might find this interesting for a riscv-laptop

Adds about ~36k or so (big for a bootloader)

Also needs the following in u-boot:

diff --git a/lib/Kconfig b/lib/Kconfig
index 622f3c26c33..38c2f6493a1 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -175,2 +175,3 @@ config AES
source lib/rsa/Kconfig
+source lib/secp256k1/Kconfig

diff --git a/lib/Makefile b/lib/Makefile
index 5f583aed37d..02ec35e8201 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -51,2 +51,3 @@ endif
obj-$(CONFIG_RSA) += rsa/
+obj-$(CONFIG_DSA) += secp256k1/
obj-$(CONFIG_SHA1) += sha1.o

Adds about ~36k or so (big for a bootloader)

diff --git a/lib/Kconfig b/lib/Kconfig
index 622f3c26c33..38c2f6493a1 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -175,2 +175,3 @@ config AES
 source lib/rsa/Kconfig
+source lib/secp256k1/Kconfig

diff --git a/lib/Makefile b/lib/Makefile
index 5f583aed37d..02ec35e8201 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -51,2 +51,3 @@ endif
 obj-$(CONFIG_RSA) += rsa/
+obj-$(CONFIG_DSA) += secp256k1/
 obj-$(CONFIG_SHA1) += sha1.o
@laanwj
Copy link
Member

laanwj commented May 7, 2019

Sorry, I didn't see this PR before. This is very interesting!

For reducing the code size, it would make sense to include only the verification parts, not signing, which would not be necessary. I don't think there is currently a define for this.

Also you're using the basic configuration,

#define USE_FIELD_10X26 1
#define USE_SCALAR_8X32 1

For RV64 you can use the 64-bit code for the field and scalar operations, which have fewer steps thus shorter (and faster):

USE_SCALAR_4X64
USE_FIELD_5X52

Note that secp256k1 precomputes a large table in the context at initialization (in secp256k1_ecmult_context_build, for faster verification). With the default settings this is about 1.25MB:

/** One table for window size 16: 1.375 MiB. */
#define WINDOW_G 16

… scales exponentially: …
#define ECMULT_TABLE_SIZE(w) (1 << ((w)-2))

For bootloader use, you'd want to a) reduce the size of the table and b) embed the table statically (which unfortunately increases the code size). It can be set arbitrarily small. I don't know what is the smallest table size that still leads to acceptable verification performance for your use case. There's a PR that makes the window configurable and has some benchmarks: #596. There hasn't been work yet on embedding the table statically AFAIK.

@gmaxwell
Copy link
Contributor

gmaxwell commented May 7, 2019

Signing uses a static table, its more or less trivial to do. (usually people who have wanted to use the library in embedded contexts have wanted signing, not verification... :) ). There are also a whole bunch of easy size optimizations where we have several different versions of a function optimized for different cases which could all be substituted by the most general.

Re leaving out signing, the linker should strip the signing parts if they're not used.

@laanwj
Copy link
Member

laanwj commented May 7, 2019

I liked the idea (also by @gmaxwell) to hardcode the table the for a few small WINDOW_G sizes. It'd not make much sense for the general case anyhow (virtually no one wants 1.5MB larger binary at the expense of a little bit less startup time), and avoids needing scripts to generate code during build.

Re leaving out signing, the linker should strip the signing parts if they're not used.

Are you sure? Unless using full program optimization, or -ffunction-sections, or a compilation unit per function (which is not the case, everything ends up in libsecp256k1_la-secp256k1.o), I don't think the linker is allowed to remove code.

@laanwj
Copy link
Member

laanwj commented May 7, 2019

I was curious so I created a list of function/data objects and their sizes—on RV64—that are used from secp256k1_context_create (verify only) and secp256k1_ecdsa_verify. This was done using -Os -ffunction-sections -fdata-sections -Wl,--gc-sections:

12    .text    secp256k1_callback_call
858   .text    secp256k1_fe_mul_inner
652   .text    secp256k1_fe_sqr_inner
188   .text    secp256k1_fe_normalize
94    .text    secp256k1_fe_normalize_weak
186   .text    secp256k1_fe_normalize_var
146   .text    secp256k1_fe_normalizes_to_zero_var
22    .text    secp256k1_fe_clear
352   .text    secp256k1_fe_set_b32
66    .text    secp256k1_fe_negate
42    .text    secp256k1_fe_mul_int
42    .text    secp256k1_fe_add
48    .text    secp256k1_fe_to_storage
66    .text    secp256k1_fe_from_storage
558   .text    secp256k1_fe_inv
86    .text    secp256k1_scalar_check_overflow
88    .text    secp256k1_scalar_reduce
288   .text    secp256k1_scalar_set_b32
250   .text    secp256k1_scalar_get_b32
20    .text    secp256k1_scalar_is_zero
150   .text    secp256k1_scalar_negate
100   .text    secp256k1_scalar_is_high
590   .text    secp256k1_scalar_reduce_512
434   .text    secp256k1_scalar_mul
454   .text    secp256k1_scalar_sqr
952   .text    secp256k1_scalar_inverse
70    .text    secp256k1_ge_set_gej_zinv
70    .text    secp256k1_gej_set_ge
76    .text    secp256k1_ge_to_storage
42    .text    secp256k1_ge_from_storage
26    .text    default_error_callback_fn
26    .text    default_illegal_callback_fn
46    .text    checked_malloc
82    .text    secp256k1_pubkey_load
280   .text    secp256k1_gej_double_var
436   .text    secp256k1_gej_add_ge_var
44    .text    secp256k1_ecdsa_signature_load.isra.0
270   .text    secp256k1_ecmult_wnaf.constprop.0
186   .text    secp256k1_ecmult_odd_multiples_table.constprop.0
1502  .text    secp256k1_ecmult
80    .text    secp256k1_gej_eq_x_var
16    .rodata  default_error_callback
40    .rodata  secp256k1_ecdsa_const_order_as_fe
40    .rodata  secp256k1_ecdsa_const_p_minus_order
88    .rodata  secp256k1_ge_const_g
496   .text    secp256k1_context_create
324   .text    secp256k1_ecdsa_verify

10984 (total)

This can be considered a lower bound on the size added to the bootloader for verification only. Of course, any hardcoded precomputed WINDOW_G table size will need to be added to this.

@gmaxwell
Copy link
Contributor

we should be able to get a 3.8% size reduction with negligible impact in performance by having something to use the strongest normalize function for all normalize uses.... but I dunno if it's worth having a define to shrink the code if that's all that it does.

Some of the larger functions like scalar inverse and fe_inverse could probably be shrank a whole lot with a small performance hit by un-unrolling them by adding a small state machine.

I notice the sqrt isn't in your list, this is because you don't have a pubkey load. The sqrt is only needed if a compressed pubkey is used. For a bootloader, you'd be best off using an uncompressed pubkey, then the code doesn't need the big and somewhat slow sqrt function.

@sipa
Copy link
Contributor

sipa commented Jul 23, 2019

What are the next steps here? The patch as-is isn't acceptable as it introduces Linux-specific includes in the source code, as well as adding a static Makefile that would be confusing to other builders. Those things should, if necessary, at least move to some contrib directory or perhaps even a separate repository.

However, there are perhaps things that can be done to the codebase itself to help its use in this setting? Maybe a way to configure while dropping things that aren't relevant to verification? Since #596 the WINDOW_G size is configurable to reduce the size of the static table.

@sipa sipa closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants