From aa04e818a4904b193e00d603785c93e888eab174 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 30 Aug 2013 10:26:15 -0700 Subject: [PATCH] Fix recursive locking bug. handle_rename() would end up acquiring the lock twice. Change to always derive has_rw inside earlier locks (instead of acquiring a second time), and pass the value into check_caller_access_to_name(). Bug: 10547597 Change-Id: If5744d6d226a4785676c19d0f7fdf1c05060ed76 --- sdcard/sdcard.c | 57 +++++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/sdcard/sdcard.c b/sdcard/sdcard.c index 7349b2241..330d555b5 100644 --- a/sdcard/sdcard.c +++ b/sdcard/sdcard.c @@ -486,12 +486,18 @@ static void derive_permissions_locked(struct fuse* fuse, struct node *parent, } } +/* Return if the calling UID holds sdcard_rw. */ +static bool get_caller_has_rw_locked(struct fuse* fuse, const struct fuse_in_header *hdr) { + appid_t appid = multiuser_get_app_id(hdr->uid); + return hashmapContainsKey(fuse->appid_with_rw, (void*) appid); +} + /* Kernel has already enforced everything we returned through * derive_permissions_locked(), so this is used to lock down access * even further, such as enforcing that apps hold sdcard_rw. */ static bool check_caller_access_to_name(struct fuse* fuse, const struct fuse_in_header *hdr, const struct node* parent_node, - const char* name, int mode) { + const char* name, int mode, bool has_rw) { /* Always block security-sensitive files at root */ if (parent_node && parent_node->perm == PERM_ROOT) { if (!strcmp(name, "autorun.inf") @@ -518,13 +524,7 @@ static bool check_caller_access_to_name(struct fuse* fuse, return true; } - appid_t appid = multiuser_get_app_id(hdr->uid); - - pthread_mutex_lock(&fuse->lock); - bool hasRw = hashmapContainsKey(fuse->appid_with_rw, (void*) appid); - pthread_mutex_unlock(&fuse->lock); - - return hasRw; + return has_rw; } /* No extra permissions to enforce */ @@ -532,8 +532,8 @@ static bool check_caller_access_to_name(struct fuse* fuse, } static bool check_caller_access_to_node(struct fuse* fuse, - const struct fuse_in_header *hdr, const struct node* node, int mode) { - return check_caller_access_to_name(fuse, hdr, node->parent, node->name, mode); + const struct fuse_in_header *hdr, const struct node* node, int mode, bool has_rw) { + return check_caller_access_to_name(fuse, hdr, node->parent, node->name, mode, has_rw); } struct node *create_node_locked(struct fuse* fuse, @@ -831,7 +831,7 @@ static int handle_lookup(struct fuse* fuse, struct fuse_handler* handler, child_path, sizeof(child_path), 1))) { return -ENOENT; } - if (!check_caller_access_to_name(fuse, hdr, parent_node, name, R_OK)) { + if (!check_caller_access_to_name(fuse, hdr, parent_node, name, R_OK, false)) { return -EACCES; } @@ -872,7 +872,7 @@ static int handle_getattr(struct fuse* fuse, struct fuse_handler* handler, if (!node) { return -ENOENT; } - if (!check_caller_access_to_node(fuse, hdr, node, R_OK)) { + if (!check_caller_access_to_node(fuse, hdr, node, R_OK, false)) { return -EACCES; } @@ -882,11 +882,13 @@ static int handle_getattr(struct fuse* fuse, struct fuse_handler* handler, static int handle_setattr(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header *hdr, const struct fuse_setattr_in *req) { + bool has_rw; struct node* node; char path[PATH_MAX]; struct timespec times[2]; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, path, sizeof(path)); TRACE("[%d] SETATTR fh=%llx valid=%x @ %llx (%s)\n", handler->token, req->fh, req->valid, hdr->nodeid, node ? node->name : "?"); @@ -895,7 +897,7 @@ static int handle_setattr(struct fuse* fuse, struct fuse_handler* handler, if (!node) { return -ENOENT; } - if (!check_caller_access_to_node(fuse, hdr, node, W_OK)) { + if (!check_caller_access_to_node(fuse, hdr, node, W_OK, has_rw)) { return -EACCES; } @@ -943,12 +945,14 @@ static int handle_setattr(struct fuse* fuse, struct fuse_handler* handler, static int handle_mknod(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const struct fuse_mknod_in* req, const char* name) { + bool has_rw; struct node* parent_node; char parent_path[PATH_MAX]; char child_path[PATH_MAX]; const char* actual_name; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); parent_node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, parent_path, sizeof(parent_path)); TRACE("[%d] MKNOD %s 0%o @ %llx (%s)\n", handler->token, @@ -959,7 +963,7 @@ static int handle_mknod(struct fuse* fuse, struct fuse_handler* handler, child_path, sizeof(child_path), 1))) { return -ENOENT; } - if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK, has_rw)) { return -EACCES; } __u32 mode = (req->mode & (~0777)) | 0664; @@ -972,12 +976,14 @@ static int handle_mknod(struct fuse* fuse, struct fuse_handler* handler, static int handle_mkdir(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const struct fuse_mkdir_in* req, const char* name) { + bool has_rw; struct node* parent_node; char parent_path[PATH_MAX]; char child_path[PATH_MAX]; const char* actual_name; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); parent_node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, parent_path, sizeof(parent_path)); TRACE("[%d] MKDIR %s 0%o @ %llx (%s)\n", handler->token, @@ -988,7 +994,7 @@ static int handle_mkdir(struct fuse* fuse, struct fuse_handler* handler, child_path, sizeof(child_path), 1))) { return -ENOENT; } - if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK, has_rw)) { return -EACCES; } __u32 mode = (req->mode & (~0777)) | 0775; @@ -1001,11 +1007,13 @@ static int handle_mkdir(struct fuse* fuse, struct fuse_handler* handler, static int handle_unlink(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const char* name) { + bool has_rw; struct node* parent_node; char parent_path[PATH_MAX]; char child_path[PATH_MAX]; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); parent_node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, parent_path, sizeof(parent_path)); TRACE("[%d] UNLINK %s @ %llx (%s)\n", handler->token, @@ -1016,7 +1024,7 @@ static int handle_unlink(struct fuse* fuse, struct fuse_handler* handler, child_path, sizeof(child_path), 1)) { return -ENOENT; } - if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK, has_rw)) { return -EACCES; } if (unlink(child_path) < 0) { @@ -1028,11 +1036,13 @@ static int handle_unlink(struct fuse* fuse, struct fuse_handler* handler, static int handle_rmdir(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const char* name) { + bool has_rw; struct node* parent_node; char parent_path[PATH_MAX]; char child_path[PATH_MAX]; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); parent_node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, parent_path, sizeof(parent_path)); TRACE("[%d] RMDIR %s @ %llx (%s)\n", handler->token, @@ -1043,7 +1053,7 @@ static int handle_rmdir(struct fuse* fuse, struct fuse_handler* handler, child_path, sizeof(child_path), 1)) { return -ENOENT; } - if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK, has_rw)) { return -EACCES; } if (rmdir(child_path) < 0) { @@ -1056,6 +1066,7 @@ static int handle_rename(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const struct fuse_rename_in* req, const char* old_name, const char* new_name) { + bool has_rw; struct node* old_parent_node; struct node* new_parent_node; struct node* child_node; @@ -1067,6 +1078,7 @@ static int handle_rename(struct fuse* fuse, struct fuse_handler* handler, int res; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); old_parent_node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, old_parent_path, sizeof(old_parent_path)); new_parent_node = lookup_node_and_path_by_id_locked(fuse, req->newdir, @@ -1079,11 +1091,11 @@ static int handle_rename(struct fuse* fuse, struct fuse_handler* handler, res = -ENOENT; goto lookup_error; } - if (!check_caller_access_to_name(fuse, hdr, old_parent_node, old_name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, old_parent_node, old_name, W_OK, has_rw)) { res = -EACCES; goto lookup_error; } - if (!check_caller_access_to_name(fuse, hdr, new_parent_node, new_name, W_OK)) { + if (!check_caller_access_to_name(fuse, hdr, new_parent_node, new_name, W_OK, has_rw)) { res = -EACCES; goto lookup_error; } @@ -1146,12 +1158,14 @@ static int open_flags_to_access_mode(int open_flags) { static int handle_open(struct fuse* fuse, struct fuse_handler* handler, const struct fuse_in_header* hdr, const struct fuse_open_in* req) { + bool has_rw; struct node* node; char path[PATH_MAX]; struct fuse_open_out out; struct handle *h; pthread_mutex_lock(&fuse->lock); + has_rw = get_caller_has_rw_locked(fuse, hdr); node = lookup_node_and_path_by_id_locked(fuse, hdr->nodeid, path, sizeof(path)); TRACE("[%d] OPEN 0%o @ %llx (%s)\n", handler->token, req->flags, hdr->nodeid, node ? node->name : "?"); @@ -1160,7 +1174,8 @@ static int handle_open(struct fuse* fuse, struct fuse_handler* handler, if (!node) { return -ENOENT; } - if (!check_caller_access_to_node(fuse, hdr, node, open_flags_to_access_mode(req->flags))) { + if (!check_caller_access_to_node(fuse, hdr, node, + open_flags_to_access_mode(req->flags), has_rw)) { return -EACCES; } h = malloc(sizeof(*h)); @@ -1307,7 +1322,7 @@ static int handle_opendir(struct fuse* fuse, struct fuse_handler* handler, if (!node) { return -ENOENT; } - if (!check_caller_access_to_node(fuse, hdr, node, R_OK)) { + if (!check_caller_access_to_node(fuse, hdr, node, R_OK, false)) { return -EACCES; } h = malloc(sizeof(*h));