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

types: Fix CK_LONG/CK_ULONG on Linux ARM32 #40

Merged
merged 2 commits into from Aug 28, 2020
Merged

types: Fix CK_LONG/CK_ULONG on Linux ARM32 #40

merged 2 commits into from Aug 28, 2020

Conversation

woodruffw
Copy link
Contributor

This fixes an ABI mismatch that I observed experimentally when trying to use rust-pkcs11 with a Nitrokey on a 32-bit ARM host (really 64-bit, but it's a Raspberry Pi running 32-bit Raspbian).

There are probably other ABI mismatches on other target OSes and architectures; a more general fix here might be to use pkcs11t.h directly via bindgen or some other mechanism.

Experimentally, this is what OpenSC's ABI uses. Using 64-bit
values here caused silent return code corruptions.
@codecov-commenter
Copy link

Codecov Report

Merging #40 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   65.43%   65.41%   -0.03%     
==========================================
  Files           4        4              
  Lines        3446     3444       -2     
==========================================
- Hits         2255     2253       -2     
  Misses       1191     1191              
Impacted Files Coverage Δ
src/types.rs 37.06% <ø> (ø)
src/lib.rs 65.12% <0.00%> (-0.04%) ⬇️
src/tests.rs 80.41% <0.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d66fee4...cc637db. Read the comment docs.

@hug-dev
Copy link
Contributor

hug-dev commented Aug 26, 2020

Wow, I have always thought that the PKCS 11 types would be typedefed over stdint.h types 😅 That is indeed an interesting problem.

I would be in favor of producing (and commiting in-tree) various bindings for a set of target triplets. Also it is worth noting @joechrisellis PR's on bindgen to be able to use libloading directly with the bindings produced.

Brainstorming here, but this PR + the need of safety given by abstraction (#38) make me wonder if it would be worth following the Rust idiom of spliting pkcs11 in a low-level pkcs11-sys crate for FFI bindings and dynamic loading and a high-level pkcs11 one for idiomatic and safe Rust types/methods.

@woodruffw
Copy link
Contributor Author

Wow, I have always thought that the PKCS 11 types would be typedefed over stdint.h types 😅 That is indeed an interesting problem.

Unfortunately not 🙃 -- they're defined over the normal C primitive types, with additional (not enforced?) assumptions about being at least 32 bits for e.g. CK_ULONG.

@mheese
Copy link
Owner

mheese commented Aug 27, 2020

good catch... and yes, bindgen is probably the right way out of this. pkcs11 is unfortunately full of "C"isms that make the life in rust really hard

@mheese
Copy link
Owner

mheese commented Aug 27, 2020

@hug-dev as you are obviously interested in ARM, would you mind researching (quickly) if we can add a build target for that?

@hug-dev
Copy link
Contributor

hug-dev commented Aug 27, 2020

@hug-dev as you are obviously interested in ARM, would you mind researching (quickly) if we can add a build target for that?

I think we should either have bindgen generating at build-time the bindings for the specific target or (better) pre-generate the bindings and choose wich one to use depending on the target.
I would say for now it is better to merge this fix and use bindgen after rust-lang/rust-bindgen#1846 is merged.
Regarding the target I believe it would be arm-unknown-linux-gnueabi?

@woodruffw
Copy link
Contributor Author

Regarding the target I believe it would be arm-unknown-linux-gnueabi?

Yes, I believe that's correct. If you need the specific variant, it's armv7l, but that shouldn't be part of the triple.

Copy link
Owner

@mheese mheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mheese mheese merged commit 98e0106 into mheese:master Aug 28, 2020
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.

None yet

4 participants