From 54672a4f1ec876cf4ae8e1b87cbb1f66698cbac0 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Fri, 15 May 2015 09:54:41 +0200 Subject: [PATCH] MDEV-8043 innodb tablespace encryption "use after free" bug, when a thread replaces space->crypt_data and frees the old crypt_data object while it's being used by another thread. --- storage/innobase/fil/fil0crypt.cc | 30 +++++++++++++++++----------- storage/innobase/fil/fil0fil.cc | 24 +++++++++++----------- storage/innobase/include/fil0crypt.h | 11 +++++----- storage/xtradb/fil/fil0crypt.cc | 30 +++++++++++++++++----------- storage/xtradb/fil/fil0fil.cc | 24 +++++++++++----------- storage/xtradb/include/fil0crypt.h | 11 +++++----- 6 files changed, 70 insertions(+), 60 deletions(-) diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc index f5d7fe98e05..1e2ca289e93 100644 --- a/storage/innobase/fil/fil0crypt.cc +++ b/storage/innobase/fil/fil0crypt.cc @@ -216,25 +216,31 @@ fil_space_create_crypt_data( } /****************************************************************** -Compare two crypt objects */ +Merge fil_space_crypt_t object */ UNIV_INTERN -int -fil_space_crypt_compare( +void +fil_space_merge_crypt_data( /*====================*/ - const fil_space_crypt_t* crypt_data1,/*!< in: Crypt data */ - const fil_space_crypt_t* crypt_data2)/*!< in: Crypt data */ + fil_space_crypt_t* dst,/*!< out: Crypt data */ + const fil_space_crypt_t* src)/*!< in: Crypt data */ { - ut_a(crypt_data1->type == CRYPT_SCHEME_UNENCRYPTED || - crypt_data1->type == CRYPT_SCHEME_1); + mutex_enter(&dst->mutex); - ut_a(crypt_data2->type == CRYPT_SCHEME_UNENCRYPTED || - crypt_data2->type == CRYPT_SCHEME_1); + /* validate that they are mergeable */ + ut_a(src->type == CRYPT_SCHEME_UNENCRYPTED || + src->type == CRYPT_SCHEME_1); + + ut_a(dst->type == CRYPT_SCHEME_UNENCRYPTED || + dst->type == CRYPT_SCHEME_1); /* no support for changing iv (yet?) */ - ut_a(memcmp(crypt_data1->iv, crypt_data2->iv, - sizeof(crypt_data1->iv)) == 0); + ut_a(memcmp(src->iv, dst->iv, sizeof(src->iv)) == 0); - return 0; + dst->type = src->type; + dst->min_key_version = src->min_key_version; + dst->keyserver_requests += src->keyserver_requests; + + mutex_exit(&dst->mutex); } /****************************************************************** diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc index 3ac16a1f647..e5fcc950587 100644 --- a/storage/innobase/fil/fil0fil.cc +++ b/storage/innobase/fil/fil0fil.cc @@ -6862,7 +6862,7 @@ fil_space_set_crypt_data( fil_space_crypt_t* crypt_data) /*!< in: crypt data */ { fil_space_t* space; - fil_space_crypt_t* old_crypt_data = NULL; + fil_space_crypt_t* free_crypt_data = NULL; ut_ad(fil_system); @@ -6872,24 +6872,24 @@ fil_space_set_crypt_data( if (space != NULL) { if (space->crypt_data != NULL) { - ut_a(!fil_space_crypt_compare(crypt_data, - space->crypt_data)); - old_crypt_data = space->crypt_data; + fil_space_merge_crypt_data(space->crypt_data, + crypt_data); + free_crypt_data = crypt_data; + } else { + space->crypt_data = crypt_data; } - - space->crypt_data = crypt_data; } else { /* there is a small risk that tablespace has been deleted */ - old_crypt_data = crypt_data; + free_crypt_data = crypt_data; } mutex_exit(&fil_system->mutex); - if (old_crypt_data != NULL) { - /* first assign space->crypt_data - * then destroy old_crypt_data when no new references to - * it can be created. + if (free_crypt_data != NULL) { + /* there was already crypt data present and the new crypt + * data provided as argument to this function has been merged + * into that => free new crypt data */ - fil_space_destroy_crypt_data(&old_crypt_data); + fil_space_destroy_crypt_data(&free_crypt_data); } } diff --git a/storage/innobase/include/fil0crypt.h b/storage/innobase/include/fil0crypt.h index 32d3c46585f..70f67daa2d2 100644 --- a/storage/innobase/include/fil0crypt.h +++ b/storage/innobase/include/fil0crypt.h @@ -141,13 +141,12 @@ fil_space_set_crypt_data( fil_space_crypt_t* crypt_data); /*!< in: crypt data to set */ /********************************************************************* -Compare crypt data*/ -UNIV_INTERN -int -fil_space_crypt_compare( +Merge crypt data */ +void +fil_space_merge_crypt_data( /*======================*/ - const fil_space_crypt_t* crypt_data1, /*!< in: crypt data */ - const fil_space_crypt_t* crypt_data2); /*!< in: crypt data */ + fil_space_crypt_t* dst_crypt_data, /*!< in: crypt_data */ + const fil_space_crypt_t* src_crypt_data); /*!< in: crypt data */ /********************************************************************* Read crypt data from buffer page */ diff --git a/storage/xtradb/fil/fil0crypt.cc b/storage/xtradb/fil/fil0crypt.cc index b64f306fb71..600d170397d 100644 --- a/storage/xtradb/fil/fil0crypt.cc +++ b/storage/xtradb/fil/fil0crypt.cc @@ -216,25 +216,31 @@ fil_space_create_crypt_data( } /****************************************************************** -Compare two crypt objects */ +Merge fil_space_crypt_t object */ UNIV_INTERN -int -fil_space_crypt_compare( +void +fil_space_merge_crypt_data( /*====================*/ - const fil_space_crypt_t* crypt_data1,/*!< in: Crypt data */ - const fil_space_crypt_t* crypt_data2)/*!< in: Crypt data */ + fil_space_crypt_t* dst,/*!< out: Crypt data */ + const fil_space_crypt_t* src)/*!< in: Crypt data */ { - ut_a(crypt_data1->type == CRYPT_SCHEME_UNENCRYPTED || - crypt_data1->type == CRYPT_SCHEME_1); + mutex_enter(&dst->mutex); - ut_a(crypt_data2->type == CRYPT_SCHEME_UNENCRYPTED || - crypt_data2->type == CRYPT_SCHEME_1); + /* validate that they are mergeable */ + ut_a(src->type == CRYPT_SCHEME_UNENCRYPTED || + src->type == CRYPT_SCHEME_1); + + ut_a(dst->type == CRYPT_SCHEME_UNENCRYPTED || + dst->type == CRYPT_SCHEME_1); /* no support for changing iv (yet?) */ - ut_a(memcmp(crypt_data1->iv, crypt_data2->iv, - sizeof(crypt_data1->iv)) == 0); + ut_a(memcmp(src->iv, dst->iv, sizeof(src->iv)) == 0); - return 0; + dst->type = src->type; + dst->min_key_version = src->min_key_version; + dst->keyserver_requests += src->keyserver_requests; + + mutex_exit(&dst->mutex); } /****************************************************************** diff --git a/storage/xtradb/fil/fil0fil.cc b/storage/xtradb/fil/fil0fil.cc index f0f92cb71eb..57f3b5fda2f 100644 --- a/storage/xtradb/fil/fil0fil.cc +++ b/storage/xtradb/fil/fil0fil.cc @@ -6989,7 +6989,7 @@ fil_space_set_crypt_data( fil_space_crypt_t* crypt_data) /*!< in: crypt data */ { fil_space_t* space; - fil_space_crypt_t* old_crypt_data = NULL; + fil_space_crypt_t* free_crypt_data = NULL; ut_ad(fil_system); @@ -6999,24 +6999,24 @@ fil_space_set_crypt_data( if (space != NULL) { if (space->crypt_data != NULL) { - ut_a(!fil_space_crypt_compare(crypt_data, - space->crypt_data)); - old_crypt_data = space->crypt_data; + fil_space_merge_crypt_data(space->crypt_data, + crypt_data); + free_crypt_data = crypt_data; + } else { + space->crypt_data = crypt_data; } - - space->crypt_data = crypt_data; } else { /* there is a small risk that tablespace has been deleted */ - old_crypt_data = crypt_data; + free_crypt_data = crypt_data; } mutex_exit(&fil_system->mutex); - if (old_crypt_data != NULL) { - /* first assign space->crypt_data - * then destroy old_crypt_data when no new references to - * it can be created. + if (free_crypt_data != NULL) { + /* there was already crypt data present and the new crypt + * data provided as argument to this function has been merged + * into that => free new crypt data */ - fil_space_destroy_crypt_data(&old_crypt_data); + fil_space_destroy_crypt_data(&free_crypt_data); } } diff --git a/storage/xtradb/include/fil0crypt.h b/storage/xtradb/include/fil0crypt.h index 32d3c46585f..70f67daa2d2 100644 --- a/storage/xtradb/include/fil0crypt.h +++ b/storage/xtradb/include/fil0crypt.h @@ -141,13 +141,12 @@ fil_space_set_crypt_data( fil_space_crypt_t* crypt_data); /*!< in: crypt data to set */ /********************************************************************* -Compare crypt data*/ -UNIV_INTERN -int -fil_space_crypt_compare( +Merge crypt data */ +void +fil_space_merge_crypt_data( /*======================*/ - const fil_space_crypt_t* crypt_data1, /*!< in: crypt data */ - const fil_space_crypt_t* crypt_data2); /*!< in: crypt data */ + fil_space_crypt_t* dst_crypt_data, /*!< in: crypt_data */ + const fil_space_crypt_t* src_crypt_data); /*!< in: crypt data */ /********************************************************************* Read crypt data from buffer page */