Make the threading code provider aware by mattcaswell · Pull Request #9040 · openssl/openssl

mattcaswell · 2019-05-29T15:27:44Z

The RAND code is thread aware and stores thread local variables. In order for that to work when its running inside the FIPS module we need to make some changes to how we clean up threads so that we can delegate some of that cleanup to the providers where appropriate.

We change the thread stop handing into a publish/subscribe model to accomplish this.

(Needed in order to move the RAND code into the FIPS provider - see #9035)

mattcaswell · 2019-05-30T10:53:23Z

Hmmm. The travis failure is annoying. A classic case of the compiler trying to be too helpful:

crypto/context.c:171:9: error: explicitly assigning value of variable of type 'OPENSSL_CTX *' (aka 'struct openssl_ctx_st *') to itself [-Werror,-Wself-assign]
    ctx = (ctx);
    ~~~ ^  ~~~

That's because of this macro definition in FIPS_MODE:

#ifdef FIPS_MODE
# define openssl_ctx_get_concrete(ctx)  (ctx)
#else
OPENSSL_CTX *openssl_ctx_get_concrete(OPENSSL_CTX *ctx);
#endif

So when I do the following it gets this error:

    ctx = openssl_ctx_get_concrete(ctx);

A no-op is exactly what I want in FIPS mode. Any bright ideas as to how I can get the compiler to shut up?

levitte · 2019-05-31T04:02:54Z

Hmmm. The travis failure is annoying. A classic case of the compiler trying to be too helpful:
crypto/context.c:171:9: error: explicitly assigning value of variable of type 'OPENSSL_CTX *' (aka 'struct openssl_ctx_st *') to itself [-Werror,-Wself-assign]
    ctx = (ctx);
    ~~~ ^  ~~~
That's because of this macro definition in FIPS_MODE:
#ifdef FIPS_MODE
# define openssl_ctx_get_concrete(ctx)  (ctx)
#else
OPENSSL_CTX *openssl_ctx_get_concrete(OPENSSL_CTX *ctx);
#endif
So when I do the following it gets this error:
    ctx = openssl_ctx_get_concrete(ctx);
A no-op is exactly what I want in FIPS mode. Any bright ideas as to how I can get the compiler to shut up?

The solution is probably to not try and be so smart. Don't create the macro in FIPS mode, and instead, wrap each call:

#ifndef FIPS_MODE
    ctx = openssl_ctx_get_concrete(ctx);
#endif

mattcaswell · 2019-05-31T09:20:11Z

The solution is probably to not try and be so smart. Don't create the macro in FIPS mode, and instead, wrap each call:

That's what I had originally - but changed it on the suggestion of @slontis.

slontis · 2019-06-01T07:13:18Z

The original comment applied to the fact that there are quite a few of these #ifdef FIPS_MODE
snippets which start making the code look quite ugly. If it doesnt work then revert back to the original.
having a new macro name that does ctx = openssl_ctx_get_concrete(ctx); in one case and then nothing in the other case would work, but it probably not be very nice either.

crypto/context.c

mattcaswell · 2019-06-03T12:52:37Z

I rebased this to resolve the conflict with master, and implemented @levitte's suggestion for openssl_ctx_get_concrete.

doc/man3/OPENSSL_init_crypto.pod

crypto/initthread.c

mattcaswell · 2019-06-03T16:39:03Z

2 fixup commits pushed addressing feedback so far.

mattcaswell · 2019-06-07T12:48:01Z

Rebased this now that #9039 went in.

Ping?

levitte

Doc nit

doc/man3/OPENSSL_CTX.pod

doc/man3/OPENSSL_init_crypto.pod

mattcaswell · 2019-06-10T12:21:56Z

Fix up commit pushed. Ping @levitte.

doc/man3/OPENSSL_CTX.pod

mattcaswell · 2019-06-11T09:09:01Z

Fix up commit pushed addressing the doc feedback comment.

mattcaswell · 2019-06-17T11:26:06Z

Rebased again due to conflicts with master!

Ping??

crypto/initthread.c

crypto/provider_core.c

include/openssl/core_numbers.h

levitte · 2019-06-17T13:31:26Z

providers/fips/fipsprov.c

Side note: if we expect other providers to do the same thing, then a non-dynamic index isn't the best of ideas... this is something I was thinking of when I originally designed the OPENSSL_CTX API and internals. It's worth giving it another deeper think, methinks...

Feel free...

mattcaswell · 2019-06-17T14:32:23Z

Updated commits pushed addressing feedback.

This adds the ability to take an OPENSSL_CTX parameter and either return it as is (unchanged), or if it is NULL return a pointer to the default ctx.

In later commits this will allow providers to subscribe to thread stop events. We will need this in the FIPS module. We also make thread stop handling OPENSSL_CTX aware (different OPENSSL_CTXs may have different thread local data that needs cleaning up).

We're going to need some of these functions in the FIPS module, but most of the rest of the code in init.c is not needed. Therefore we split it out.

This will need to be hooked up in a later commit with an event sent to the FIPS provider informing it of thread stop events.

The RAND code needs to know about threads stopping in order to cleanup local thread data. Therefore we add a callback for libcrypto to tell providers about such events.

This adds the ability to clean up a thread on a per OPENSSL_CTX basis.

This new function works in the same way as OPENSSL_thread_stop() but for a specified OPENSSL_CTX.

mattcaswell · 2019-06-17T14:38:22Z

...and immediately I've rebased it again due to conflicts with master....grrrr....

levitte · 2019-06-17T14:47:48Z

include/openssl/core.h

 */
 # define OSSL_PARAM_OCTET_PTR            7

+/* Typedef for thread stop handling callback */


Considering the rest of this header file, that comment seems a bit... minimal

Fixup pushed expanding out that comment.

include/openssl/core_numbers.h

mattcaswell · 2019-06-17T15:25:39Z

Pushed. Thanks!!

This adds the ability to take an OPENSSL_CTX parameter and either return it as is (unchanged), or if it is NULL return a pointer to the default ctx. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

In later commits this will allow providers to subscribe to thread stop events. We will need this in the FIPS module. We also make thread stop handling OPENSSL_CTX aware (different OPENSSL_CTXs may have different thread local data that needs cleaning up). Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

We're going to need some of these functions in the FIPS module, but most of the rest of the code in init.c is not needed. Therefore we split it out. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

This will need to be hooked up in a later commit with an event sent to the FIPS provider informing it of thread stop events. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

The RAND code needs to know about threads stopping in order to cleanup local thread data. Therefore we add a callback for libcrypto to tell providers about such events. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

This adds the ability to clean up a thread on a per OPENSSL_CTX basis. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

This new function works in the same way as OPENSSL_thread_stop() but for a specified OPENSSL_CTX. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

mattcaswell added the branch: master Applies to master branch label May 29, 2019

mattcaswell mentioned this pull request May 29, 2019

Make the RAND code available from within the FIPS module #9035

Closed

levitte requested changes Jun 3, 2019

View reviewed changes

crypto/context.c Outdated Show resolved Hide resolved

mattcaswell force-pushed the provider-threads branch from 9bc6074 to 04f96ff Compare June 3, 2019 12:51

levitte requested changes Jun 3, 2019

View reviewed changes

doc/man3/OPENSSL_init_crypto.pod Outdated Show resolved Hide resolved

crypto/initthread.c Outdated Show resolved Hide resolved

mattcaswell force-pushed the provider-threads branch from 0746905 to 7645b8c Compare June 7, 2019 12:45

levitte requested changes Jun 7, 2019

View reviewed changes

doc/man3/OPENSSL_CTX.pod Outdated Show resolved Hide resolved

doc/man3/OPENSSL_init_crypto.pod Outdated Show resolved Hide resolved

doc/man3/OPENSSL_init_crypto.pod Outdated Show resolved Hide resolved

paulidale reviewed Jun 11, 2019

View reviewed changes

doc/man3/OPENSSL_CTX.pod Outdated Show resolved Hide resolved

mattcaswell force-pushed the provider-threads branch from 5080278 to 69bae9f Compare June 17, 2019 11:25

levitte requested changes Jun 17, 2019

View reviewed changes

crypto/initthread.c Outdated Show resolved Hide resolved

crypto/initthread.c Outdated Show resolved Hide resolved

crypto/provider_core.c Outdated Show resolved Hide resolved

levitte reviewed Jun 17, 2019

View reviewed changes

include/openssl/core_numbers.h Outdated Show resolved Hide resolved

levitte reviewed Jun 17, 2019

View reviewed changes

mattcaswell added 7 commits June 17, 2019 15:32

Add the function openssl_ctx_get_concrete()

8ffb582

This adds the ability to take an OPENSSL_CTX parameter and either return it as is (unchanged), or if it is NULL return a pointer to the default ctx.

Split thread intialisation and handling out of init.c

50c725d

We're going to need some of these functions in the FIPS module, but most of the rest of the code in init.c is not needed. Therefore we split it out.

Provide a version of ossl_init_thread_start that works in FIPS mode

6947844

This will need to be hooked up in a later commit with an event sent to the FIPS provider informing it of thread stop events.

Tell the FIPS provider about thread stop events

2abbdb0

The RAND code needs to know about threads stopping in order to cleanup local thread data. Therefore we add a callback for libcrypto to tell providers about such events.

Add the function OPENSSL_thread_stop_ex()

e74f9c4

This adds the ability to clean up a thread on a per OPENSSL_CTX basis.

Document OPENSSL_thread_stop_ex()

0789cd4

This new function works in the same way as OPENSSL_thread_stop() but for a specified OPENSSL_CTX.

mattcaswell added 2 commits June 17, 2019 15:33

fixup! Tell the FIPS provider about thread stop events

540e747

Standardise the function naming conventions in initthread.c

37c9ab4

mattcaswell force-pushed the provider-threads branch from b25e1fe to 37c9ab4 Compare June 17, 2019 14:37

levitte requested changes Jun 17, 2019

View reviewed changes

fixup! Tell the FIPS provider about thread stop events

80daad0

levitte approved these changes Jun 17, 2019

View reviewed changes

mattcaswell closed this Jun 17, 2019

pull bot pushed a commit to Mattlk13/openssl-1 that referenced this pull request Jun 17, 2019

Add the function OPENSSL_thread_stop_ex()

52b18ce

This adds the ability to clean up a thread on a per OPENSSL_CTX basis. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

pull bot pushed a commit to Mattlk13/openssl-1 that referenced this pull request Jun 17, 2019

Standardise the function naming conventions in initthread.c

2be8c56

Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#9040)

Uh oh!

Conversation

mattcaswell commented May 29, 2019

Uh oh!

mattcaswell commented May 30, 2019

Uh oh!

levitte commented May 31, 2019

Uh oh!

mattcaswell commented May 31, 2019

Uh oh!

slontis commented Jun 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Jun 3, 2019

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Jun 3, 2019

Uh oh!

mattcaswell commented Jun 7, 2019

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Jun 10, 2019

Uh oh!

Uh oh!

mattcaswell commented Jun 11, 2019

Uh oh!

mattcaswell commented Jun 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Jun 17, 2019

Uh oh!

mattcaswell commented Jun 17, 2019

Uh oh!

levitte Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattcaswell commented Jun 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

slontis commented Jun 1, 2019 •

edited

Loading