From ed13819a0d197d6851dca623707344ef2cad850b Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 1 May 2017 22:30:39 -0700 Subject: fuse_sideload: Change the minimal block size to 4096. run_fuse_sideload() is passing the block size as the max_read option, so it will only handle a request that involves at most two blocks at a time. However, the minimal allowed value was set to 1024 prior to this CL, which is inconsistent with the kernel code (fs/fuse/inode.c) that sets it to the greater of 4096 and the passed-in max_read option. This would fail the calls with a block size / max_read less than 4096 due to the wrongly computed block indices. Note that we didn't observe real issue in practice, because we have been using 64 KiB block sizes for both of adb and sdcard sideload calls. The issue only shows up in my local CL (to come later) that uses 1024 block size in run_fuse_sideload() tests. Test: recovery_component_test Test: adb sideload with the new recovery image on angler Change-Id: Id9f0cfea13d0d193dcb7cd41a1553a23739545f2 --- Android.mk | 4 +- fuse_sideload.cpp | 278 +++++++++++++++++++------------------- tests/Android.mk | 1 + tests/component/sideload_test.cpp | 19 ++- 4 files changed, 161 insertions(+), 141 deletions(-) diff --git a/Android.mk b/Android.mk index 5ce5cd7dc..fbb0c665d 100644 --- a/Android.mk +++ b/Android.mk @@ -25,7 +25,9 @@ LOCAL_SRC_FILES := fuse_sideload.cpp LOCAL_CFLAGS := -Wall -Werror LOCAL_CFLAGS += -D_XOPEN_SOURCE -D_GNU_SOURCE LOCAL_MODULE := libfusesideload -LOCAL_STATIC_LIBRARIES := libcrypto +LOCAL_STATIC_LIBRARIES := \ + libcrypto \ + libbase include $(BUILD_STATIC_LIBRARY) # libmounts (static library) diff --git a/fuse_sideload.cpp b/fuse_sideload.cpp index 279a976ad..219374fdb 100644 --- a/fuse_sideload.cpp +++ b/fuse_sideload.cpp @@ -61,6 +61,9 @@ #include #include +#include + +#include #include #include "fuse_sideload.h" @@ -364,164 +367,163 @@ static int handle_read(void* data, struct fuse_data* fd, const struct fuse_in_he return NO_STATUS; } -int run_fuse_sideload(struct provider_vtab* vtab, void* cookie, - uint64_t file_size, uint32_t block_size) -{ - int result; - - // If something's already mounted on our mountpoint, try to remove - // it. (Mostly in case of a previous abnormal exit.) - umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_FORCE); - - if (block_size < 1024) { - fprintf(stderr, "block size (%u) is too small\n", block_size); - return -1; - } - if (block_size > (1<<22)) { // 4 MiB - fprintf(stderr, "block size (%u) is too large\n", block_size); - return -1; - } - - struct fuse_data fd; - memset(&fd, 0, sizeof(fd)); - fd.vtab = vtab; - fd.cookie = cookie; - fd.file_size = file_size; - fd.block_size = block_size; - fd.file_blocks = (file_size == 0) ? 0 : (((file_size-1) / block_size) + 1); - - if (fd.file_blocks > (1<<18)) { - fprintf(stderr, "file has too many blocks (%u)\n", fd.file_blocks); - result = -1; - goto done; - } - - fd.hashes = (uint8_t*)calloc(fd.file_blocks, SHA256_DIGEST_LENGTH); - if (fd.hashes == NULL) { - fprintf(stderr, "failed to allocate %d bites for hashes\n", - fd.file_blocks * SHA256_DIGEST_LENGTH); - result = -1; - goto done; - } - - fd.uid = getuid(); - fd.gid = getgid(); - - fd.curr_block = -1; - fd.block_data = (uint8_t*)malloc(block_size); - if (fd.block_data == NULL) { - fprintf(stderr, "failed to allocate %d bites for block_data\n", block_size); - result = -1; - goto done; +int run_fuse_sideload(struct provider_vtab* vtab, void* cookie, uint64_t file_size, + uint32_t block_size) { + // If something's already mounted on our mountpoint, try to remove it. (Mostly in case of a + // previous abnormal exit.) + umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_FORCE); + + // fs/fuse/inode.c in kernel code uses the greater of 4096 and the passed-in max_read. + if (block_size < 4096) { + fprintf(stderr, "block size (%u) is too small\n", block_size); + return -1; + } + if (block_size > (1 << 22)) { // 4 MiB + fprintf(stderr, "block size (%u) is too large\n", block_size); + return -1; + } + + struct fuse_data fd = {}; + fd.vtab = vtab; + fd.cookie = cookie; + fd.file_size = file_size; + fd.block_size = block_size; + fd.file_blocks = (file_size == 0) ? 0 : (((file_size - 1) / block_size) + 1); + + int result; + if (fd.file_blocks > (1 << 18)) { + fprintf(stderr, "file has too many blocks (%u)\n", fd.file_blocks); + result = -1; + goto done; + } + + fd.hashes = (uint8_t*)calloc(fd.file_blocks, SHA256_DIGEST_LENGTH); + if (fd.hashes == NULL) { + fprintf(stderr, "failed to allocate %d bites for hashes\n", + fd.file_blocks * SHA256_DIGEST_LENGTH); + result = -1; + goto done; + } + + fd.uid = getuid(); + fd.gid = getgid(); + + fd.curr_block = -1; + fd.block_data = (uint8_t*)malloc(block_size); + if (fd.block_data == NULL) { + fprintf(stderr, "failed to allocate %d bites for block_data\n", block_size); + result = -1; + goto done; + } + fd.extra_block = (uint8_t*)malloc(block_size); + if (fd.extra_block == NULL) { + fprintf(stderr, "failed to allocate %d bites for extra_block\n", block_size); + result = -1; + goto done; + } + + fd.ffd = open("/dev/fuse", O_RDWR); + if (fd.ffd < 0) { + perror("open /dev/fuse"); + result = -1; + goto done; + } + + { + std::string opts = android::base::StringPrintf( + "fd=%d,user_id=%d,group_id=%d,max_read=%u,allow_other,rootmode=040000", fd.ffd, fd.uid, + fd.gid, block_size); + + result = mount("/dev/fuse", FUSE_SIDELOAD_HOST_MOUNTPOINT, "fuse", + MS_NOSUID | MS_NODEV | MS_RDONLY | MS_NOEXEC, opts.c_str()); + if (result < 0) { + perror("mount"); + goto done; } - fd.extra_block = (uint8_t*)malloc(block_size); - if (fd.extra_block == NULL) { - fprintf(stderr, "failed to allocate %d bites for extra_block\n", block_size); + } + + uint8_t request_buffer[sizeof(struct fuse_in_header) + PATH_MAX * 8]; + for (;;) { + ssize_t len = TEMP_FAILURE_RETRY(read(fd.ffd, request_buffer, sizeof(request_buffer))); + if (len == -1) { + perror("read request"); + if (errno == ENODEV) { result = -1; - goto done; + break; + } + continue; } - fd.ffd = open("/dev/fuse", O_RDWR); - if (fd.ffd < 0) { - perror("open /dev/fuse"); - result = -1; - goto done; + if (static_cast(len) < sizeof(struct fuse_in_header)) { + fprintf(stderr, "request too short: len=%zd\n", len); + continue; } - char opts[256]; - snprintf(opts, sizeof(opts), - ("fd=%d,user_id=%d,group_id=%d,max_read=%u," - "allow_other,rootmode=040000"), - fd.ffd, fd.uid, fd.gid, block_size); - - result = mount("/dev/fuse", FUSE_SIDELOAD_HOST_MOUNTPOINT, - "fuse", MS_NOSUID | MS_NODEV | MS_RDONLY | MS_NOEXEC, opts); - if (result < 0) { - perror("mount"); - goto done; - } - uint8_t request_buffer[sizeof(struct fuse_in_header) + PATH_MAX*8]; - for (;;) { - ssize_t len = TEMP_FAILURE_RETRY(read(fd.ffd, request_buffer, sizeof(request_buffer))); - if (len == -1) { - perror("read request"); - if (errno == ENODEV) { - result = -1; - break; - } - continue; - } - - if ((size_t)len < sizeof(struct fuse_in_header)) { - fprintf(stderr, "request too short: len=%zu\n", (size_t)len); - continue; - } + struct fuse_in_header* hdr = reinterpret_cast(request_buffer); + void* data = request_buffer + sizeof(struct fuse_in_header); - struct fuse_in_header* hdr = (struct fuse_in_header*) request_buffer; - void* data = request_buffer + sizeof(struct fuse_in_header); + result = -ENOSYS; - result = -ENOSYS; + switch (hdr->opcode) { + case FUSE_INIT: + result = handle_init(data, &fd, hdr); + break; - switch (hdr->opcode) { - case FUSE_INIT: - result = handle_init(data, &fd, hdr); - break; + case FUSE_LOOKUP: + result = handle_lookup(data, &fd, hdr); + break; - case FUSE_LOOKUP: - result = handle_lookup(data, &fd, hdr); - break; + case FUSE_GETATTR: + result = handle_getattr(data, &fd, hdr); + break; - case FUSE_GETATTR: - result = handle_getattr(data, &fd, hdr); - break; + case FUSE_OPEN: + result = handle_open(data, &fd, hdr); + break; - case FUSE_OPEN: - result = handle_open(data, &fd, hdr); - break; + case FUSE_READ: + result = handle_read(data, &fd, hdr); + break; - case FUSE_READ: - result = handle_read(data, &fd, hdr); - break; + case FUSE_FLUSH: + result = handle_flush(data, &fd, hdr); + break; - case FUSE_FLUSH: - result = handle_flush(data, &fd, hdr); - break; + case FUSE_RELEASE: + result = handle_release(data, &fd, hdr); + break; - case FUSE_RELEASE: - result = handle_release(data, &fd, hdr); - break; - - default: - fprintf(stderr, "unknown fuse request opcode %d\n", hdr->opcode); - break; - } + default: + fprintf(stderr, "unknown fuse request opcode %d\n", hdr->opcode); + break; + } - if (result == NO_STATUS_EXIT) { - result = 0; - break; - } + if (result == NO_STATUS_EXIT) { + result = 0; + break; + } - if (result != NO_STATUS) { - struct fuse_out_header outhdr; - outhdr.len = sizeof(outhdr); - outhdr.error = result; - outhdr.unique = hdr->unique; - TEMP_FAILURE_RETRY(write(fd.ffd, &outhdr, sizeof(outhdr))); - } + if (result != NO_STATUS) { + struct fuse_out_header outhdr; + outhdr.len = sizeof(outhdr); + outhdr.error = result; + outhdr.unique = hdr->unique; + TEMP_FAILURE_RETRY(write(fd.ffd, &outhdr, sizeof(outhdr))); } + } - done: - fd.vtab->close(fd.cookie); +done: + fd.vtab->close(fd.cookie); - result = umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_DETACH); - if (result < 0) { - printf("fuse_sideload umount failed: %s\n", strerror(errno)); - } + result = umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_DETACH); + if (result < 0) { + printf("fuse_sideload umount failed: %s\n", strerror(errno)); + } - if (fd.ffd) close(fd.ffd); - free(fd.hashes); - free(fd.block_data); - free(fd.extra_block); + if (fd.ffd) close(fd.ffd); + free(fd.hashes); + free(fd.block_data); + free(fd.extra_block); - return result; + return result; } diff --git a/tests/Android.mk b/tests/Android.mk index 4e125ccce..02a240127 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -126,6 +126,7 @@ LOCAL_STATIC_LIBRARIES := \ libimgpatch \ libbsdiff \ libbspatch \ + libfusesideload \ libotafault \ librecovery \ libupdater \ diff --git a/tests/component/sideload_test.cpp b/tests/component/sideload_test.cpp index ea93e9b84..40cfc6975 100644 --- a/tests/component/sideload_test.cpp +++ b/tests/component/sideload_test.cpp @@ -13,9 +13,24 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + #include + #include -TEST(SideloadTest, fusedevice) { - ASSERT_NE(-1, access("/dev/fuse", R_OK | W_OK)); +#include "fuse_sideload.h" + +TEST(SideloadTest, fuse_device) { + ASSERT_EQ(0, access("/dev/fuse", R_OK | W_OK)); +} + +TEST(SideloadTest, run_fuse_sideload_wrong_parameters) { + provider_vtab vtab; + vtab.close = [](void*) {}; + + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, 4096, 4095)); + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, 4096, (1 << 22) + 1)); + + // Too many blocks. + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, ((1 << 18) + 1) * 4096, 4096)); } -- cgit v1.2.3