From ab5fedda0bf6869ce9d9f0b633766d7c307286c3 Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Fri, 16 Nov 2018 03:20:30 -0500 Subject: [PATCH] Prevent Magisk database race condition The database should only be accessed by a single process, which is magiskd. This means 'magisk --sqlite [SQL]' has to be updated to pass the SQL command to the daemon. In addition, open the database connection with SQLITE_OPEN_FULLMUTEX to support multithread in magiskd. --- native/jni/daemon/bootstages.cpp | 16 ++-- native/jni/daemon/daemon.cpp | 6 ++ native/jni/daemon/db.cpp | 128 ++++++++++++--------------- native/jni/daemon/magisk.cpp | 6 +- native/jni/include/daemon.h | 3 +- native/jni/include/db.h | 18 ++-- native/jni/magiskhide/hide_utils.cpp | 61 +++++-------- native/jni/su/su_daemon.cpp | 42 ++++----- 8 files changed, 130 insertions(+), 150 deletions(-) diff --git a/native/jni/daemon/bootstages.cpp b/native/jni/daemon/bootstages.cpp index f31f1a7dc..d7591d1d7 100644 --- a/native/jni/daemon/bootstages.cpp +++ b/native/jni/daemon/bootstages.cpp @@ -884,16 +884,12 @@ core_only: install_apk("/data/magisk.apk"); } else { // Check whether we have a valid manager installed - sqlite3 *db = get_magiskdb(); - if (db) { - db_strings str; - get_db_strings(db, &str, SU_MANAGER); - if (validate_manager(str[SU_MANAGER], 0, nullptr)) { - // There is no manager installed, install the stub - exec_command_sync("/sbin/magiskinit", "-x", "manager", "/data/magisk.apk", nullptr); - install_apk("/data/magisk.apk"); - } - sqlite3_close_v2(db); + db_strings str; + get_db_strings(&str, SU_MANAGER); + if (validate_manager(str[SU_MANAGER], 0, nullptr)) { + // There is no manager installed, install the stub + exec_command_sync("/sbin/magiskinit", "-x", "manager", "/data/magisk.apk", nullptr); + install_apk("/data/magisk.apk"); } } diff --git a/native/jni/daemon/daemon.cpp b/native/jni/daemon/daemon.cpp index 83cf5f1cc..cc4ab1e9a 100644 --- a/native/jni/daemon/daemon.cpp +++ b/native/jni/daemon/daemon.cpp @@ -18,6 +18,7 @@ #include "utils.h" #include "daemon.h" #include "selinux.h" +#include "db.h" #include "flags.h" static void get_client_cred(int fd, struct ucred *cred) { @@ -39,6 +40,7 @@ static void *request_handler(void *args) { case POST_FS_DATA: case LATE_START: case BOOT_COMPLETE: + case SQLITE_CMD: if (credential.uid != 0) { write_int(client, ROOT_REQUIRED); close(client); @@ -75,6 +77,10 @@ static void *request_handler(void *args) { case HANDSHAKE: /* Do NOT close the client, make it hold */ break; + case SQLITE_CMD: + exec_sql(client); + close(client); + break; default: close(client); break; diff --git a/native/jni/daemon/db.cpp b/native/jni/daemon/db.cpp index 379d2ec84..f0a1769e8 100644 --- a/native/jni/daemon/db.cpp +++ b/native/jni/daemon/db.cpp @@ -7,9 +7,12 @@ #include "magisk.h" #include "db.h" +#include "daemon.h" #define DB_VERSION 7 +static sqlite3 *mDB = nullptr; + db_strings::db_strings() { memset(data, 0, sizeof(data)); } @@ -76,24 +79,17 @@ static int ver_cb(void *ver, int, char **data, char **) { return 0; } -#define err_abort(err) \ -if (err) { \ - LOGE("sqlite3_exec: %s\n", err); \ - sqlite3_free(err); \ - return nullptr; \ -} +#define err_ret(e) if (e) return e; -static sqlite3 *open_and_init_db() { - sqlite3 *db; - int ret = sqlite3_open(MAGISKDB, &db); - if (ret) { - LOGE("sqlite3 open failure: %s\n", sqlite3_errstr(ret)); - return nullptr; - } +static char *open_and_init_db(sqlite3 *&db) { + int ret = sqlite3_open_v2(MAGISKDB, &db, + SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX, nullptr); + if (ret) + return strdup(sqlite3_errstr(ret)); int ver, upgrade = 0; char *err; sqlite3_exec(db, "PRAGMA user_version", ver_cb, &ver, &err); - err_abort(err); + err_ret(err); if (ver > DB_VERSION) { // Don't support downgrading database sqlite3_close_v2(db); @@ -106,20 +102,20 @@ static sqlite3 *open_and_init_db() { "(uid INT, package_name TEXT, policy INT, until INT, " "logging INT, notification INT, PRIMARY KEY(uid))", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); // Logs sqlite3_exec(db, "CREATE TABLE IF NOT EXISTS logs " "(from_uid INT, package_name TEXT, app_name TEXT, from_pid INT, " "to_uid INT, action INT, time INT, command TEXT)", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); // Settings sqlite3_exec(db, "CREATE TABLE IF NOT EXISTS settings " "(key TEXT, value INT, PRIMARY KEY(key))", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); ver = 3; upgrade = 1; } @@ -129,13 +125,13 @@ static sqlite3 *open_and_init_db() { "CREATE TABLE IF NOT EXISTS strings " "(key TEXT, value TEXT, PRIMARY KEY(key))", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); ver = 4; upgrade = 1; } if (ver == 4) { sqlite3_exec(db, "UPDATE policies SET uid=uid%100000", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); /* Skip version 5 */ ver = 6; upgrade = 1; @@ -146,7 +142,7 @@ static sqlite3 *open_and_init_db() { "CREATE TABLE IF NOT EXISTS hidelist " "(process TEXT, PRIMARY KEY(process))", nullptr, nullptr, &err); - err_abort(err); + err_ret(err); ver = 7; upgrade =1 ; } @@ -156,19 +152,27 @@ static sqlite3 *open_and_init_db() { char query[32]; sprintf(query, "PRAGMA user_version=%d", ver); sqlite3_exec(db, query, nullptr, nullptr, &err); - err_abort(err); + err_ret(err); } - return db; + return nullptr; } -sqlite3 *get_magiskdb() { - sqlite3 *db = open_and_init_db(); - if (db == nullptr) { - // Open fails, remove and reconstruct - unlink(MAGISKDB); - db = open_and_init_db(); +char *db_exec(const char *sql, int (*cb)(void *, int, char**, char**), void *v) { + char *err; + if (mDB == nullptr) { + err = open_and_init_db(mDB); + db_err_cmd(err, + // Open fails, remove and reconstruct + unlink(MAGISKDB); + err = open_and_init_db(mDB); + err_ret(err); + ); } - return db; + if (mDB) { + sqlite3_exec(mDB, sql, cb, v, &err); + return err; + } + return nullptr; } static int settings_cb(void *v, int col_num, char **data, char **col_name) { @@ -189,22 +193,16 @@ static int settings_cb(void *v, int col_num, char **data, char **col_name) { return 0; } -int get_db_settings(sqlite3 *db, db_settings *dbs, int key) { - if (db == nullptr) - return 1; +int get_db_settings(db_settings *dbs, int key) { char *err; if (key > 0) { char query[128]; sprintf(query, "SELECT key, value FROM settings WHERE key='%s'", DB_SETTING_KEYS[key]); - sqlite3_exec(db, query, settings_cb, dbs, &err); + err = db_exec(query, settings_cb, dbs); } else { - sqlite3_exec(db, "SELECT key, value FROM settings", settings_cb, dbs, &err); - } - if (err) { - LOGE("sqlite3_exec: %s\n", err); - sqlite3_free(err); - return 1; + err = db_exec("SELECT key, value FROM settings", settings_cb, dbs); } + db_err_cmd(err, return 1); return 0; } @@ -225,16 +223,14 @@ static int strings_cb(void *v, int col_num, char **data, char **col_name) { return 0; } -int get_db_strings(sqlite3 *db, db_strings *str, int key) { - if (db == nullptr) - return 1; +int get_db_strings(db_strings *str, int key) { char *err; if (key > 0) { char query[128]; sprintf(query, "SELECT key, value FROM strings WHERE key='%s'", DB_STRING_KEYS[key]); - sqlite3_exec(db, query, strings_cb, str, &err); + err = db_exec(query, strings_cb, str); } else { - sqlite3_exec(db, "SELECT key, value FROM strings", strings_cb, str, &err); + err = db_exec("SELECT key, value FROM strings", strings_cb, str); } if (err) { LOGE("sqlite3_exec: %s\n", err); @@ -258,18 +254,12 @@ static int policy_cb(void *v, int col_num, char **data, char **col_name) { return 0; } -int get_uid_policy(sqlite3 *db, int uid, struct su_access *su) { - if (db == nullptr) - return 1; +int get_uid_policy(int uid, struct su_access *su) { char query[256], *err; sprintf(query, "SELECT policy, logging, notification FROM policies " "WHERE uid=%d AND (until=0 OR until>%li)", uid, time(nullptr)); - sqlite3_exec(db, query, policy_cb, su, &err); - if (err) { - LOGE("sqlite3_exec: %s\n", err); - sqlite3_free(err); - return 1; - } + err = db_exec(query, policy_cb, su); + db_err_cmd(err, return 1); return 0; } @@ -299,26 +289,24 @@ int validate_manager(char *alt_pkg, int userid, struct stat *st) { } static int print_cb(void *v, int col_num, char **data, char **col_name) { + FILE *out = (FILE *) v; for (int i = 0; i < col_num; ++i) { - if (i) printf("|"); - printf("%s=%s", col_name[i], data[i]); + if (i) fprintf(out, "|"); + fprintf(out, "%s=%s", col_name[i], data[i]); } - printf("\n"); + fprintf(out, "\n"); return 0; } -int exec_sql(const char *sql) { - sqlite3 *db = get_magiskdb(); - if (db) { - char *err; - sqlite3_exec(db, sql, print_cb, nullptr, &err); - sqlite3_close_v2(db); - if (err) { - fprintf(stderr, "sql_err: %s\n", err); - sqlite3_free(err); - return 1; - } - return 0; - } - return 1; +void exec_sql(int client) { + char *sql = read_string(client); + FILE *out = fdopen(recv_fd(client), "a"); + char *err = db_exec(sql, print_cb, out); + free(sql); + fclose(out); + db_err_cmd(err, + write_int(client, 1); + return; + ); + write_int(client, 0); } diff --git a/native/jni/daemon/magisk.cpp b/native/jni/daemon/magisk.cpp index ac9489613..65257b3e9 100644 --- a/native/jni/daemon/magisk.cpp +++ b/native/jni/daemon/magisk.cpp @@ -113,7 +113,11 @@ int magisk_main(int argc, char *argv[]) { write_int(fd, BOOT_COMPLETE); return read_int(fd); } else if (strcmp(argv[1], "--sqlite") == 0) { - return exec_sql(argv[2]); + int fd = connect_daemon(); + write_int(fd, SQLITE_CMD); + write_string(fd, argv[2]); + send_fd(fd, STDOUT_FILENO); + return read_int(fd); } usage(); diff --git a/native/jni/include/daemon.h b/native/jni/include/daemon.h index 7588cd512..b2c468d71 100644 --- a/native/jni/include/daemon.h +++ b/native/jni/include/daemon.h @@ -20,7 +20,8 @@ enum { BOOT_COMPLETE, MAGISKHIDE, HIDE_CONNECT, - HANDSHAKE + HANDSHAKE, + SQLITE_CMD, }; // Return codes for daemon diff --git a/native/jni/include/db.h b/native/jni/include/db.h index fe6fdd9d6..1b69d4019 100644 --- a/native/jni/include/db.h +++ b/native/jni/include/db.h @@ -4,6 +4,13 @@ #include #include +#define db_err(e) db_err_cmd(e, ) +#define db_err_cmd(e, cmd) if (e) { \ + LOGE("sqlite3_exec: %s\n", e); \ + sqlite3_free(e); \ + cmd;\ +} + /*************** * DB Settings * ***************/ @@ -128,11 +135,12 @@ struct su_access { * Public Functions * ********************/ -sqlite3 *get_magiskdb(); -int get_db_settings(sqlite3 *db, db_settings *dbs, int key = -1); -int get_db_strings(sqlite3 *db, db_strings *str, int key = -1); -int get_uid_policy(sqlite3 *db, int uid, struct su_access *su); +sqlite3 *get_magiskdb(sqlite3 *&db); +int get_db_settings(db_settings *dbs, int key); +int get_db_strings(db_strings *str, int key); +int get_uid_policy(int uid, struct su_access *su); int validate_manager(char *alt_pkg, int userid, struct stat *st); -int exec_sql(const char *sql); +void exec_sql(int client); +char *db_exec(const char *sql, int (*cb)(void *, int, char**, char**) = nullptr, void *v = nullptr); #endif //DB_H diff --git a/native/jni/magiskhide/hide_utils.cpp b/native/jni/magiskhide/hide_utils.cpp index 539ece8da..025203f99 100644 --- a/native/jni/magiskhide/hide_utils.cpp +++ b/native/jni/magiskhide/hide_utils.cpp @@ -140,7 +140,7 @@ void clean_magisk_props() { }, nullptr, false); } -static int add_list(sqlite3 *db, const char *proc) { +int add_list(const char *proc) { for (auto &s : hide_list) { // They should be unique if (s == proc) @@ -149,30 +149,21 @@ static int add_list(sqlite3 *db, const char *proc) { LOGI("hide_list add: [%s]\n", proc); + // Add to database + char sql[4096]; + snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", proc); + char *err = db_exec(sql); + db_err_cmd(err, return DAEMON_ERROR); + // Critical region pthread_mutex_lock(&list_lock); hide_list.push_back(proc); kill_process(proc); pthread_mutex_unlock(&list_lock); - // Add to database - char sql[4096]; - snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", proc); - sqlite3_exec(db, sql, nullptr, nullptr, nullptr); - return DAEMON_SUCCESS; } -int add_list(const char *proc) { - sqlite3 *db = get_magiskdb(); - if (db) { - int ret = add_list(db, proc); - sqlite3_close_v2(db); - return ret; - } - return DAEMON_ERROR; -} - int add_list(int client) { char *proc = read_string(client); int ret = add_list(proc); @@ -196,13 +187,10 @@ static int rm_list(const char *proc) { pthread_mutex_unlock(&list_lock); if (do_rm) { - sqlite3 *db = get_magiskdb(); - if (db == nullptr) - return DAEMON_ERROR; char sql[4096]; snprintf(sql, sizeof(sql), "DELETE FROM hidelist WHERE process='%s'", proc); - sqlite3_exec(db, sql, nullptr, nullptr, nullptr); - sqlite3_close_v2(db); + char *err = db_exec(sql); + db_err(err); return DAEMON_SUCCESS; } else { return HIDE_ITEM_NOT_EXIST; @@ -218,31 +206,27 @@ int rm_list(int client) { #define LEGACY_LIST MOUNTPOINT "/.core/hidelist" +static int collect_list(void *, int, char **data, char**) { + LOGI("hide_list: [%s]\n", data[0]); + hide_list.push_back(data[0]); + return 0; +} + bool init_list() { LOGD("hide_list: initialize\n"); - sqlite3 *db = get_magiskdb(); - if (db == nullptr) - return false; - - sqlite3_exec(db, "SELECT process FROM hidelist", - [] (auto, auto, char **data, auto) -> int - { - LOGI("hide_list: [%s]\n", data[0]); - hide_list.push_back(data[0]); - return 0; - }, nullptr, nullptr); + char *err = db_exec("SELECT process FROM hidelist", collect_list); + db_err_cmd(err, return false); // Migrate old hide list into database if (access(LEGACY_LIST, R_OK) == 0) { Vector tmp; file_to_vector(LEGACY_LIST, tmp); for (auto &s : tmp) - add_list(db, s); + add_list(s); unlink(LEGACY_LIST); } - sqlite3_close_v2(db); return true; } @@ -256,12 +240,11 @@ void ls_list(int client) { } static void set_hide_config() { - sqlite3 *db = get_magiskdb(); char sql[64]; sprintf(sql, "REPLACE INTO settings (key,value) VALUES('%s',%d)", DB_SETTING_KEYS[HIDE_CONFIG], hide_enabled); - sqlite3_exec(db, sql, nullptr, nullptr, nullptr); - sqlite3_close_v2(db); + char *err = db_exec(sql); + db_err(err); } int launch_magiskhide(int client) { @@ -314,10 +297,8 @@ int stop_magiskhide() { void auto_start_magiskhide() { if (!start_log_daemon()) return; - sqlite3 *db = get_magiskdb(); db_settings dbs; - get_db_settings(db, &dbs, HIDE_CONFIG); - sqlite3_close_v2(db); + get_db_settings(&dbs, HIDE_CONFIG); if (dbs[HIDE_CONFIG]) { pthread_t thread; xpthread_create(&thread, nullptr, [](void*) -> void* { diff --git a/native/jni/su/su_daemon.cpp b/native/jni/su/su_daemon.cpp index b947d88a2..243d34e83 100644 --- a/native/jni/su/su_daemon.cpp +++ b/native/jni/su/su_daemon.cpp @@ -60,32 +60,28 @@ static void *info_collector(void *node) { static void database_check(su_info *info) { int uid = info->uid; - sqlite3 *db = get_magiskdb(); - if (db) { - get_db_settings(db, &info->cfg); - get_db_strings(db, &info->str); + get_db_settings(&info->cfg, 0); + get_db_strings(&info->str, 0); - // Check multiuser settings - switch (info->cfg[SU_MULTIUSER_MODE]) { - case MULTIUSER_MODE_OWNER_ONLY: - if (info->uid / 100000) { - uid = -1; - info->access = NO_SU_ACCESS; - } - break; - case MULTIUSER_MODE_OWNER_MANAGED: - uid = info->uid % 100000; - break; - case MULTIUSER_MODE_USER: - default: - break; - } - - if (uid > 0) - get_uid_policy(db, uid, &info->access); - sqlite3_close_v2(db); + // Check multiuser settings + switch (info->cfg[SU_MULTIUSER_MODE]) { + case MULTIUSER_MODE_OWNER_ONLY: + if (info->uid / 100000) { + uid = -1; + info->access = NO_SU_ACCESS; + } + break; + case MULTIUSER_MODE_OWNER_MANAGED: + uid = info->uid % 100000; + break; + case MULTIUSER_MODE_USER: + default: + break; } + if (uid > 0) + get_uid_policy(uid, &info->access); + // We need to check our manager if (info->access.log || info->access.notify) validate_manager(info->str[SU_MANAGER], uid / 100000, &info->mgr_st);