Merge "Prepare necessary data directory before loading persistent properties." am: 399bd0866c am: e1f216cc4d am: cbe1f6b1e4
am: faec65d7c3
Change-Id: Ie8e0bcc05bdf83351b3bfb73309c1b00eb511e30
diff --git a/Android.mk b/Android.mk
index 06d98eb..8a1ffa0 100644
--- a/Android.mk
+++ b/Android.mk
@@ -69,6 +69,11 @@
libbatteryservice \
libavb \
+# TODO: include "cert-err34-c" once we move to Binder
+# TODO: include "cert-err58-cpp" once 36656327 is fixed
+common_local_tidy_flags := -warnings-as-errors=clang-analyzer-security*,cert-*
+common_local_tidy_checks := -*,clang-analyzer-security*,cert-*,-cert-err34-c,-cert-err58-cpp
+
vold_conlyflags := -std=c11
vold_cflags := -Werror -Wall -Wno-missing-field-initializers -Wno-unused-variable -Wno-unused-parameter
@@ -87,6 +92,9 @@
LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
LOCAL_MODULE := libvold
LOCAL_CLANG := true
+LOCAL_TIDY := true
+LOCAL_TIDY_FLAGS := $(common_local_tidy_flags)
+LOCAL_TIDY_CHECKS := $(common_local_tidy_checks)
LOCAL_SRC_FILES := $(common_src_files)
LOCAL_C_INCLUDES := $(common_c_includes)
LOCAL_SHARED_LIBRARIES := $(common_shared_libraries)
@@ -103,6 +111,9 @@
LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
LOCAL_MODULE := vold
LOCAL_CLANG := true
+LOCAL_TIDY := true
+LOCAL_TIDY_FLAGS := $(common_local_tidy_flags)
+LOCAL_TIDY_CHECKS := $(common_local_tidy_checks)
LOCAL_SRC_FILES := \
main.cpp \
$(common_src_files)
@@ -123,6 +134,9 @@
LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
LOCAL_CLANG := true
+LOCAL_TIDY := true
+LOCAL_TIDY_FLAGS := $(common_local_tidy_flags)
+LOCAL_TIDY_CHECKS := $(common_local_tidy_checks)
LOCAL_SRC_FILES := vdc.cpp
LOCAL_MODULE := vdc
LOCAL_SHARED_LIBRARIES := libcutils libbase
@@ -136,6 +150,9 @@
LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk
LOCAL_CLANG := true
+LOCAL_TIDY := true
+LOCAL_TIDY_FLAGS := $(common_local_tidy_flags)
+LOCAL_TIDY_CHECKS := $(common_local_tidy_checks)
LOCAL_SRC_FILES:= secdiscard.cpp
LOCAL_MODULE:= secdiscard
LOCAL_SHARED_LIBRARIES := libbase
diff --git a/CommandListener.cpp b/CommandListener.cpp
index c2b8310..78973db 100644
--- a/CommandListener.cpp
+++ b/CommandListener.cpp
@@ -327,8 +327,8 @@
continue;
}
- char processName[255];
- Process::getProcessName(pid, processName, sizeof(processName));
+ std::string processName;
+ Process::getProcessName(pid, processName);
if (Process::checkFileDescriptorSymLinks(pid, argv[2]) ||
Process::checkFileMaps(pid, argv[2]) ||
@@ -337,7 +337,7 @@
Process::checkSymLink(pid, argv[2], "exe")) {
char msg[1024];
- snprintf(msg, sizeof(msg), "%d %s", pid, processName);
+ snprintf(msg, sizeof(msg), "%d %s", pid, processName.c_str());
cli->sendMsg(ResponseCode::StorageUsersListResult, msg, false);
}
}
diff --git a/Devmapper.cpp b/Devmapper.cpp
index e56a4da..4b6942d 100644
--- a/Devmapper.cpp
+++ b/Devmapper.cpp
@@ -195,7 +195,7 @@
char *geoParams = buffer + sizeof(struct dm_ioctl);
// bps=512 spc=8 res=32 nft=2 sec=8190 mid=0xf0 spt=63 hds=64 hid=0 bspf=8 rdcl=2 infs=1 bkbs=2
- strcpy(geoParams, "0 64 63 0");
+ strlcpy(geoParams, "0 64 63 0", DEVMAPPER_BUFFER_SIZE - sizeof(struct dm_ioctl));
geoParams += strlen(geoParams) + 1;
geoParams = (char *) _align(geoParams, 8);
if (ioctl(fd, DM_DEV_SET_GEOMETRY, io)) {
diff --git a/Ext4Crypt.cpp b/Ext4Crypt.cpp
index 59b76d6..9dfcad8 100644
--- a/Ext4Crypt.cpp
+++ b/Ext4Crypt.cpp
@@ -20,14 +20,11 @@
#include "Utils.h"
#include <algorithm>
-#include <chrono>
#include <iomanip>
#include <map>
-#include <mutex>
#include <set>
#include <sstream>
#include <string>
-#include <thread>
#include <dirent.h>
#include <errno.h>
@@ -63,7 +60,6 @@
static constexpr int FLAG_STORAGE_CE = 1 << 1;
namespace {
-const std::chrono::seconds s_key_eviction_sleep_time(20);
const std::string device_key_dir = std::string() + DATA_MNT_POINT + e4crypt_unencrypted_folder;
const std::string device_key_path = device_key_dir + "/key";
@@ -77,10 +73,6 @@
// Some users are ephemeral, don't try to wipe their keys from disk
std::set<userid_t> s_ephemeral_users;
-// Allow evictions to be cancelled.
-std::map<std::string, std::thread::id> s_desired_eviction_threads;
-std::mutex s_desired_eviction_threads_mutex;
-
// Map user ids to key references
std::map<userid_t, std::string> s_de_key_raw_refs;
std::map<userid_t, std::string> s_ce_key_raw_refs;
@@ -167,9 +159,6 @@
ext4_encryption_key ext4_key;
if (!fill_key(key, &ext4_key)) return false;
*raw_ref = generate_key_ref(ext4_key.raw, ext4_key.size);
- // Ensure that no thread is waiting to evict this ref
- std::lock_guard<std::mutex> lock(s_desired_eviction_threads_mutex);
- s_desired_eviction_threads.erase(*raw_ref);
auto ref = keyname(*raw_ref);
key_serial_t device_keyring;
if (!e4crypt_keyring(&device_keyring)) return false;
@@ -537,43 +526,22 @@
return true;
}
-static void evict_key_after_delay(const std::string raw_ref) {
- LOG(DEBUG) << "Waiting to evict key in thread " << std::this_thread::get_id();
- std::this_thread::sleep_for(s_key_eviction_sleep_time);
- LOG(DEBUG) << "Done waiting to evict key in thread " << std::this_thread::get_id();
-
- std::lock_guard<std::mutex> lock(s_desired_eviction_threads_mutex);
- // Check the key should be evicted by this thread
- auto search = s_desired_eviction_threads.find(raw_ref);
- if (search == s_desired_eviction_threads.end()) {
- LOG(DEBUG) << "Not evicting renewed-desirability key";
- return;
- }
- if (search->second != std::this_thread::get_id()) {
- LOG(DEBUG) << "We are not the thread to evict this key";
- return;
- }
-
- // Remove the key from the keyring
+static bool evict_key(const std::string &raw_ref) {
auto ref = keyname(raw_ref);
key_serial_t device_keyring;
- if (!e4crypt_keyring(&device_keyring)) return;
+ if (!e4crypt_keyring(&device_keyring)) return false;
auto key_serial = keyctl_search(device_keyring, "logon", ref.c_str(), 0);
- if (keyctl_revoke(key_serial) != 0) {
- PLOG(ERROR) << "Failed to revoke key with serial " << key_serial;
- return;
- }
- LOG(DEBUG) << "Revoked key with serial " << key_serial;
-}
-static bool evict_key(const std::string &raw_ref) {
- // FIXME the use of a thread with delay is a work around for a kernel bug
- std::lock_guard<std::mutex> lock(s_desired_eviction_threads_mutex);
- std::thread t(evict_key_after_delay, raw_ref);
- s_desired_eviction_threads[raw_ref] = t.get_id();
- LOG(DEBUG) << "Scheduled key eviction in thread " << t.get_id();
- t.detach();
- return true; // Sadly no way to know if we were successful :(
+ // Unlink the key from the keyring. Prefer unlinking to revoking or
+ // invalidating, since unlinking is actually no less secure currently, and
+ // it avoids bugs in certain kernel versions where the keyring key is
+ // referenced from places it shouldn't be.
+ if (keyctl_unlink(key_serial, device_keyring) != 0) {
+ PLOG(ERROR) << "Failed to unlink key with serial " << key_serial << " ref " << ref;
+ return false;
+ }
+ LOG(DEBUG) << "Unlinked key with serial " << key_serial << " ref " << ref;
+ return true;
}
static bool evict_ce_key(userid_t user_id) {
diff --git a/Process.cpp b/Process.cpp
index fd757d5..1c0f504 100644
--- a/Process.cpp
+++ b/Process.cpp
@@ -28,10 +28,17 @@
#include <signal.h>
#define LOG_TAG "ProcessKiller"
+
+#include <android-base/file.h>
+#include <android-base/stringprintf.h>
+#include <android-base/logging.h>
#include <cutils/log.h>
#include "Process.h"
+using android::base::ReadFileToString;
+using android::base::StringPrintf;
+
int Process::readSymLink(const char *path, char *link, size_t max) {
struct stat s;
int length;
@@ -40,10 +47,10 @@
return 0;
if ((s.st_mode & S_IFMT) != S_IFLNK)
return 0;
-
- // we have a symlink
+
+ // we have a symlink
length = readlink(path, link, max- 1);
- if (length <= 0)
+ if (length <= 0)
return 0;
link[length] = 0;
return 1;
@@ -63,16 +70,9 @@
return 0;
}
-void Process::getProcessName(int pid, char *buffer, size_t max) {
- int fd;
- snprintf(buffer, max, "/proc/%d/cmdline", pid);
- fd = open(buffer, O_RDONLY | O_CLOEXEC);
- if (fd < 0) {
- strcpy(buffer, "???");
- } else {
- int length = read(fd, buffer, max - 1);
- buffer[length] = 0;
- close(fd);
+void Process::getProcessName(int pid, std::string& out_name) {
+ if (!ReadFileToString(StringPrintf("/proc/%d/cmdline", pid), &out_name)) {
+ out_name = "???";
}
}
@@ -103,7 +103,7 @@
// append the file name, after truncating to parent directory
path[parent_length] = 0;
- strcat(path, de->d_name);
+ strlcat(path, de->d_name, PATH_MAX);
char link[PATH_MAX];
@@ -189,24 +189,24 @@
while ((de = readdir(dir))) {
int pid = getPid(de->d_name);
- char name[PATH_MAX];
-
if (pid == -1)
continue;
- getProcessName(pid, name, sizeof(name));
+
+ std::string name;
+ getProcessName(pid, name);
char openfile[PATH_MAX];
if (checkFileDescriptorSymLinks(pid, path, openfile, sizeof(openfile))) {
- SLOGE("Process %s (%d) has open file %s", name, pid, openfile);
+ SLOGE("Process %s (%d) has open file %s", name.c_str(), pid, openfile);
} else if (checkFileMaps(pid, path, openfile, sizeof(openfile))) {
- SLOGE("Process %s (%d) has open filemap for %s", name, pid, openfile);
+ SLOGE("Process %s (%d) has open filemap for %s", name.c_str(), pid, openfile);
} else if (checkSymLink(pid, path, "cwd")) {
- SLOGE("Process %s (%d) has cwd within %s", name, pid, path);
+ SLOGE("Process %s (%d) has cwd within %s", name.c_str(), pid, path);
} else if (checkSymLink(pid, path, "root")) {
- SLOGE("Process %s (%d) has chroot within %s", name, pid, path);
+ SLOGE("Process %s (%d) has chroot within %s", name.c_str(), pid, path);
} else if (checkSymLink(pid, path, "exe")) {
- SLOGE("Process %s (%d) has executable path within %s", name, pid, path);
+ SLOGE("Process %s (%d) has executable path within %s", name.c_str(), pid, path);
} else {
continue;
}
diff --git a/Process.h b/Process.h
index 62a9313..4ddbdb9 100644
--- a/Process.h
+++ b/Process.h
@@ -28,7 +28,7 @@
static int checkFileMaps(int pid, const char *path, char *openFilename, size_t max);
static int checkFileDescriptorSymLinks(int pid, const char *mountPoint);
static int checkFileDescriptorSymLinks(int pid, const char *mountPoint, char *openFilename, size_t max);
- static void getProcessName(int pid, char *buffer, size_t max);
+ static void getProcessName(int pid, std::string& out_name);
private:
static int readSymLink(const char *path, char *link, size_t max);
static int pathMatchesMountPoint(const char *path, const char *mountPoint);
diff --git a/Utils.cpp b/Utils.cpp
index 443df1d..050a92b 100644
--- a/Utils.cpp
+++ b/Utils.cpp
@@ -292,7 +292,7 @@
LOG(ERROR) << "Failed to setexeccon";
abort();
}
- FILE* fp = popen(cmd.c_str(), "r");
+ FILE* fp = popen(cmd.c_str(), "r"); // NOLINT
if (setexeccon(nullptr)) {
LOG(ERROR) << "Failed to setexeccon";
abort();
diff --git a/VolumeManager.cpp b/VolumeManager.cpp
index 3b4c054..e038303 100644
--- a/VolumeManager.cpp
+++ b/VolumeManager.cpp
@@ -180,7 +180,7 @@
}
*createdDMDevice = true;
} else {
- strcpy(buffer, loopDevice);
+ strlcpy(buffer, loopDevice, len);
*createdDMDevice = false;
}
return 0;
@@ -931,7 +931,7 @@
cleanupDm = true;
} else {
sb.c_cipher = ASEC_SB_C_CIPHER_NONE;
- strcpy(dmDevice, loopDevice);
+ strlcpy(dmDevice, loopDevice, sizeof(dmDevice));
}
/*
@@ -1895,7 +1895,7 @@
// Create a string to compare against that has a trailing slash
int loopDirLen = strlen(VolumeManager::LOOPDIR);
char loopDir[loopDirLen + 2];
- strcpy(loopDir, VolumeManager::LOOPDIR);
+ strlcpy(loopDir, VolumeManager::LOOPDIR, sizeof(loopDir));
loopDir[loopDirLen++] = '/';
loopDir[loopDirLen] = '\0';
diff --git a/cryptfs.cpp b/cryptfs.cpp
index f2f0f18..5d1453f 100644
--- a/cryptfs.cpp
+++ b/cryptfs.cpp
@@ -1725,7 +1725,8 @@
memset(&ext_crypt_ftr, 0, sizeof(ext_crypt_ftr));
ext_crypt_ftr.fs_size = nr_sec;
ext_crypt_ftr.keysize = keysize;
- strcpy((char*) ext_crypt_ftr.crypto_type_name, "aes-cbc-essiv:sha256");
+ strlcpy((char*) ext_crypt_ftr.crypto_type_name, "aes-cbc-essiv:sha256",
+ MAX_CRYPTO_TYPE_NAME_LEN);
return create_crypto_blk_dev(&ext_crypt_ftr, key, real_blkdev,
out_crypto_blkdev, label);
@@ -2238,7 +2239,7 @@
}
}
- if (setjmp(setjmp_env)) {
+ if (setjmp(setjmp_env)) { // NOLINT
SLOGE("Reading ext4 extent caused an exception\n");
rc = -1;
goto errout;