Page MenuHomeFreeBSD

ossl: fix OPENSSL_ia32cap_P size mismatch on i386/amd64
Needs ReviewPublic

Authored by seuros on Thu, Aug 13, 2:10 PM.
Tags
None
Referenced Files
F168279139: D58824.id.diff
Thu, Aug 27, 9:01 AM
Unknown Object (File)
Mon, Aug 24, 5:52 AM
Unknown Object (File)
Fri, Aug 21, 9:59 AM
Unknown Object (File)
Thu, Aug 13, 2:16 PM
Unknown Object (File)
Thu, Aug 13, 2:14 PM
Subscribers

Details

Reviewers
adrian
imp
Summary

Array was 4 words; vendor asm .comm requests 10, so linker warns
and the reserved capability bits are never seen by the C side.

Diff Detail

Repository
rG FreeBSD src repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 75563
Build 72446: arc lint + arc unit

Event Timeline

seuros created this revision.

https://cgit.freebsd.org/src/tree/sys/crypto/openssl/i386/x86cpuid.S << this is vendored.

Maybe i should write : upstream ? Or it fine ?

Indeed, the assembly in crypto/openssl/crypto/x86_64cpuid.pl has:

.hidden OPENSSL_ia32cap_P
.comm   OPENSSL_ia32cap_P,40,4  # <--Should match with internal/cryptlib.h OPENSSL_IA32CAP_P_MAX_INDEXES
.text

and crypto/openssl/include/internal/cryptlib.h has:

#if defined(__i386) || defined(__i386__) || defined(_M_IX86) || defined(__x86_64) || defined(__x86_64__) || defined(_M_AMD64) || defined(_M_X64)
#define OPENSSL_IA32CAP_P_MAX_INDEXES 10
extern unsigned int OPENSSL_ia32cap_P[];
#endif

It's unfortunate we can't just use the OPENSSL_IA32CAP_P_MAX_INDEXES define directly in sys/crypto/openssl/ossl_x86.c, but fixing the value directly is probably better than leaving it too small.

Alternatively, since it's a common symbol, we might also get away with declaring it as weak, and not specifying the size at all? But for kernel land, I'm unsure whether this works.

The code in sys/crypto/openssl/ossl_x86.c got moved around by @mhorne in rGfd86ae6800383dabe050e22176783857895800e3, maybe Mitchell has opinions about this too.

In D58824#1349990, @dim wrote:

The code in sys/crypto/openssl/ossl_x86.c got moved around by @mhorne in rGfd86ae6800383dabe050e22176783857895800e3, maybe Mitchell has opinions about this too.

Sorry, I have no idea!

Ok, the original ossl.c with this OPENSSL_ia32cap_P definition was added by @jhb in rGba610be90a7cb6d851e0e0e6d7612769352a3c0c. This was in 2020, so maybe OPENSSL_ia32cap_P was indeed 4. The code in ossl_cpuid() only accesses OPENSSL_ia32cap_P[0] through OPENSSL_ia32cap_P[3], not anything beyond it.

The original code was correct at that time. I only noticed this warning after compiling FreeBSD for the Xbox.

sys/crypto/openssl/ossl_x86.c
44

Note this, and see the comments below. If we are going to expand the array, it's important that we populate the new leaves with the correct values as documented in the updated manpage for newer versions of OpenSSL.

The current manpage (https://man.freebsd.org/cgi/man.cgi?query=OPENSSL_ia32cap&apropos=0&sektion=0&manpath=FreeBSD+16.0-CURRENT&format=html) for OpenSSL in the tree only documents one additional level (LV4). Presumably the other leaves are just zero-initialized.

If the assembly is setting the size by treating it as a common symbol, then perhaps we can remove this definition and instead just depend on it being declared extern as an array. That I think is a modification of what dim@ suggested and would let us use OpenSSL's size always (which is more future proof). I guess for now this commit can just remove the declaration below (and replace it with an extern if needed).

Implementing the LV4 leaf could be a separate commit, but is worth doing.