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

Find common code #873

Closed
wants to merge 4 commits into from
Closed

Find common code #873

wants to merge 4 commits into from

Conversation

baentsch
Copy link
Member

@baentsch baentsch commented Dec 16, 2020

Partial fix for #849 (edit)

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the the list of algorithms available -- either adding or removing? (If so, PRs in OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also be required by the time this is merged.)

@baentsch baentsch requested a review from dstebila December 16, 2020 10:30
@baentsch
Copy link
Member Author

@dstebila Please take a look at the new script find_common_code.sh: It needs to be run in the main liboqs folder after having executed a build. It then scans for symbols in the common space and tries to determine and output where those are used within src/kem and src/sig. It's probably erring on the conservative side (outputting too many references), but it's a start for (automating) #849 .

@dstebila dstebila changed the title initial proposal to resolve #849 Find common code Dec 17, 2020
@baentsch
Copy link
Member Author

baentsch commented Feb 3, 2021

"Ready for Review" intentionally not clicked to avoid this PR closing #849 automatically before agreement as to what's needed to close it.

@dstebila dstebila added this to the 0.5.1 RC1 milestone Feb 24, 2021
@baentsch
Copy link
Member Author

Latest addition scours all object files, trying to "manually" resolve them and find candidate symbols for putting into common code (appearing more often --5 times currently-- as unresolved), eliminating known libc, openssl and common OQS symbols. Example run output:

$ python3 tests/collect-objects.py  build
known symbols: 6344; unknown symbols: 4969
Library candidate (unknown in 6 locations): seedexpander
Library candidate (unknown in 6 locations): seedexpander_init
Symbol not found: OQS_KEM_bike1_l3_fo_bike_errno 
Library candidate (unknown in 14 locations): __tls_get_addr
Symbol not found: __tls_get_addr 
Symbol not found: OQS_KEM_bike1_l3_cpa_bike_errno 
Symbol not found: OQS_KEM_bike1_l1_fo_bike_errno 
Symbol not found: OQS_KEM_bike1_l1_cpa_bike_errno 
Symbol not found: RAND_bytes 
Symbol not found: ERR_print_errors_fp 
Symbol not found: OQS_MEM_aligned_alloc 
Symbol not found: OQS_MEM_aligned_free 
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_absorb
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_finalize
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_init
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_squeezeblocks
Symbol not found: lowmc_128_128_20 
Symbol not found: lowmc_129_129_4 
Symbol not found: lowmc_192_192_30 
Symbol not found: lowmc_192_192_4 
Symbol not found: lowmc_255_255_4 
Symbol not found: lowmc_256_256_38 

This provides a final aid to resolve #849 (short of manually checking all code for possible "common code candidates").

@baentsch baentsch marked this pull request as ready for review April 14, 2021 09:41
@baentsch baentsch requested a review from xvzcf as a code owner April 14, 2021 09:41
@dstebila
Copy link
Member

dstebila commented Apr 16, 2021

Library candidate (unknown in 6 locations): seedexpander
Library candidate (unknown in 6 locations): seedexpander_init
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_absorb
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_finalize
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_init
Library candidate (unknown in 6 locations): pqcrystals_fips202_ref_shake256_squeezeblocks

Thanks Michael!

I looked at seedexpander, and this is a very thin wrapper around our existing AES functions, so nothing significant to refactor there.

I'm a bit confused about the pqcrystals_fips202_ref_shake256_absorb ones. As far as I can tell, those are coming up in the Dilithium source code. Now, there's some macro renaming going on there which is concatenating pqcrystals_fips202_ref_ in front of shake256_absorb. I can see calls to pqcrystals_fips202_ref_shake256_absorb existing in the Dilithium source code, but I can't actually find anywhere that the symbol pqcrystals_fips202_ref_shake256_absorb is implemented, nor do I see a shim mapping pqcrystals_fips202_ref_shake256_absorb onto an OQS function, so I don't actually understand how it is that the build isn't failing.

@jschanck
Copy link
Contributor

@dstebila the pqcrystals symbols are showing up because mb-issue849 diverged from main sometime in December. You'll get different results after a git rebase.

@dstebila
Copy link
Member

dstebila commented Apr 16, 2021

@dstebila the pqcrystals symbols are showing up because mb-issue849 diverged from main sometime in December. You'll get different results after a git rebase.

Thanks John, done.

The only symbols I see of potential interest now are those in the src/sig/dilitihium/pqcrystals-dilithium_common directories.

@baentsch Can you take a look to see if you see any other symbols of interest? If not, then I'd suggest we make an issue specifically about that and close this and #849.

@baentsch
Copy link
Member Author

Can you take a look to see if you see any other symbols of interest?

Nope.

we make an issue specifically about that and close this and #849 .

Done: #973

I did not yet delete the branch yet in case we'd like to retain the "symbols-scouring" scripts. But given they've been only "quick-and-dirty" tools to investigate #849 we could also delete them/the branch.

@baentsch baentsch closed this Apr 19, 2021
@dstebila dstebila deleted the mb-issue849 branch July 7, 2021 14:11
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.

3 participants