Skip to content

make local functions in ecp_nistz256-sparcv9.S indeed local#9132

Closed
vladak wants to merge 1 commit intoopenssl:masterfrom
vladak:sparcv9_ecp_ztext
Closed

make local functions in ecp_nistz256-sparcv9.S indeed local#9132
vladak wants to merge 1 commit intoopenssl:masterfrom
vladak:sparcv9_ecp_ztext

Conversation

@vladak
Copy link
Contributor

@vladak vladak commented Jun 11, 2019

Fixes a linking issue on Solaris 11.4 by making all local functions to adhere to common scheme.

I used the solaris64-sparcv9-cc build target and used Oracle Developer studio 12.5 + Solaris linker for building. e.g. libcrypto.so was built like this:

cc -KPIC -xarch=v9 -xstrconst -Xa -xO5 -xdepend -L. -G -dy -z text -Wl,-Bsymbolic -mt  -Wl,-h,libcrypto.so.3 \
        -o libcrypto.so.3 -Wl,-M,libcrypto.ld crypto/aes/libcrypto-shlib-aes-sparcv9.o  
... number of object files elided
                 -lsocket -lnsl -ldl -lpthread -lc

i.e. it used the -z text option that uncovered the problematic ecp_nistz256_point_add_vis3 function definition.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 11, 2019
@vladak
Copy link
Contributor Author

vladak commented Jun 11, 2019

Ideally this should be backported to 1.1.x branch.

@levitte
Copy link
Member

levitte commented Jun 11, 2019

So a name scheme makes a difference? 'cause -z text is about avoiding relocation updates, isn't it? I'm surprised a different names makes a difference...

@vladak
Copy link
Contributor Author

vladak commented Jun 11, 2019 •
edited
Loading

The main fix is removing .globl from ecp_nistz256_point_add_vis3 (https://github.com/openssl/openssl/pull/9132/files#diff-3b7038c490042dde66c6db803498a808L2304), the naming scheme just makes it nice and tidy.

@levitte
Copy link
Member

levitte commented Jun 11, 2019

Oh! Didn't see the .globl change...

How about you reduce the change to just that, since that's the significant bit?

@vladak vladak force-pushed the sparcv9_ecp_ztext branch from fb65352 to a771566 Compare June 11, 2019 14:28
@vladak
Copy link
Contributor Author

vladak commented Jun 11, 2019

I split this into 2 changesets.

@vladak
Copy link
Contributor Author

vladak commented Jun 12, 2019

The reason the fix works is quite simple: if the symbol is global, it could be overridden by another library so linker needs to cater for this, hence the relocation. There is very good explanation on https://eli.thegreenplace.net/2011/08/25/load-time-relocation-of-shared-libraries/ under the 'Extra credit' paragraph.

For completeness, the object file was compiled like so:

cc  -I. -Icrypto/include -Iinclude -Icrypto -KPIC  -Xa -DSOLARIS_OPENSSL -Qoption cg -xregs=no%appl -xO5 -xstrconst -xdepend -m64 -xspace -DB_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-1.1\"" -D_REENTRANT -DNDEBUG  -c -o crypto/ec/ecp_nistz256-sparcv9.o crypto/ec/ecp_nistz256-sparcv9.S

Notably, the compiler uses -KPIC. So, if the ecp_nistz256_point_add_vis3() function was invoked via call instruction rather than be instruction the problem would not exist.

@vladak
Copy link
Contributor Author

vladak commented Jun 12, 2019

Individual CLA was submitted.

@paulidale
Copy link
Contributor

paulidale commented Jun 12, 2019

The Oracle CCLA will need to be updated too.

@vladak
Copy link
Contributor Author

vladak commented Jun 12, 2019 via email

@richsalz
Copy link
Contributor

richsalz commented Jun 12, 2019

Note that the CCLA does not need to be changed, just an updated "Schedule A" with the right list of names. The CLA file in the internal repo has the contact name for each company.

@vladak
Copy link
Contributor Author

vladak commented Jun 21, 2019

I sent the individual CLA more than a week ago. Is this waiting on this or rather the corporate CLA ?

@paulidale paulidale added branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: master Applies to master branch labels Jun 21, 2019
@paulidale
Copy link
Contributor

paulidale commented Jun 21, 2019

The corporate CLA list of people who can contribute needs to have your name added to it.
The changes also require two approvals (one now).

Both of these preventing merging.

@levitte
Copy link
Member

levitte commented Jun 22, 2019

I'm still going to be picky about the internal symbol name changes. If that is to be done, it should happen across all sparc assembly files, not just one seemingly picked at random. That's an argument to make that kind of cleanup in a separate PR.

@vladak vladak force-pushed the sparcv9_ecp_ztext branch from a771566 to e77791f Compare June 24, 2019 08:18
@vladak
Copy link
Contributor Author

vladak commented Jun 24, 2019

All right, I dropped the changeset and will file a new issue to track the naming changes.

@vladak vladak mentioned this pull request Jun 24, 2019
Closed
@levitte
Copy link
Member

levitte commented Jun 24, 2019

Thanks for obliging

@vladak
Copy link
Contributor Author

vladak commented Aug 8, 2019

closing to kick the CLA bot

@vladak vladak closed this Aug 8, 2019
@vladak vladak reopened this Aug 8, 2019
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Aug 8, 2019
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Aug 9, 2019
@mattcaswell
Copy link
Member

mattcaswell commented Aug 9, 2019

Pushed to master and 1.1.1. Thanks.

@mattcaswell mattcaswell closed this Aug 9, 2019
levitte pushed a commit that referenced this pull request Aug 9, 2019
@vladak @mattcaswell
make ecp_nistz256_point_add_vis3() local
c762f5f 
fixes #8936

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #9132)

(cherry picked from commit 8613350)
levitte pushed a commit that referenced this pull request Aug 9, 2019
@vladak @mattcaswell
make ecp_nistz256_point_add_vis3() local
8613350 
fixes #8936

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #9132)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vladak @levitte @paulidale @richsalz @mattcaswell @openssl-machine