From 798b94518e61ced3f7be7766727705df4859878c Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 7 Aug 2017 16:00:25 +0200 Subject: [PATCH] ubi tools: ubiupdatevol supports "-" input and actually respects -s SIZE Decided to not make any flash applets NOEXEC. Minor robustifications here and there. Better error messages. Save on strings: function old new delta ubi_tools_main 1235 1288 +53 ubi_get_volid_by_name 125 133 +8 ubirename_main 198 204 +6 get_num_from_file 90 94 +4 ------------------------------------------------------------------------------ (add/remove: 0/0 grow/shrink: 4/0 up/down: 71/0) Total: 71 bytes text data bss dec hex filename 915696 485 6880 923061 e15b5 busybox_old 915670 485 6880 923035 e159b busybox_unstripped Signed-off-by: Denys Vlasenko --- NOFORK_NOEXEC.lst | 22 ++++++------ libbb/ubi.c | 1 + miscutils/flash_eraseall.c | 1 + miscutils/flash_lock_unlock.c | 1 + miscutils/flashcp.c | 1 + miscutils/ubi_tools.c | 63 ++++++++++++++++++++++------------- miscutils/ubirename.c | 6 +++- 7 files changed, 59 insertions(+), 36 deletions(-) diff --git a/NOFORK_NOEXEC.lst b/NOFORK_NOEXEC.lst index d54c206fe..981a10192 100644 --- a/NOFORK_NOEXEC.lst +++ b/NOFORK_NOEXEC.lst @@ -123,10 +123,10 @@ fgconsole - noexec. leaks: get_console_fd_or_die() may open a new fd, or return fgrep - longterm runner ("CMD | fgrep ..." may run indefinitely, better to exec to conserve memory) find - noexec. runner findfs - suid -flash_eraseall -flash_lock -flash_unlock -flashcp - needs ^C. flash writing may be slow, better to free memory by execing +flash_eraseall - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +flash_lock - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +flash_unlock - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +flashcp - needs ^C. could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) flock - spawner, changes state (file locks), let's play safe and not be noexec fold - noexec. runner free - nofork candidate(struct globals, needs to close /proc/meminfo fd) @@ -366,13 +366,13 @@ tty - NOFORK ttysize - NOFORK tunctl - noexec tune2fs - noexec. leaks: open+xfunc -ubiattach -ubidetach -ubimkvol -ubirename -ubirmvol -ubirsvol -ubiupdatevol +ubiattach - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubidetach - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubimkvol - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubirename - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubirmvol - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubirsvol - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) +ubiupdatevol - could be noexec, but I feel flash ops are risky (prone to hw/fw/sw bugs) udhcpc - daemon udhcpd - daemon udpsvd - daemon diff --git a/libbb/ubi.c b/libbb/ubi.c index 34595d797..a90016acf 100644 --- a/libbb/ubi.c +++ b/libbb/ubi.c @@ -35,6 +35,7 @@ int FAST_FUNC ubi_get_volid_by_name(unsigned ubi_devnum, const char *vol_name) if (open_read_close(fname, buf, sizeof(buf)) <= 0) continue; + buf[UBI_MAX_VOLUME_NAME] = '\0'; strchrnul(buf, '\n')[0] = '\0'; if (strcmp(vol_name, buf) == 0) return i; diff --git a/miscutils/flash_eraseall.c b/miscutils/flash_eraseall.c index af9ebea24..3ddd9dd99 100644 --- a/miscutils/flash_eraseall.c +++ b/miscutils/flash_eraseall.c @@ -17,6 +17,7 @@ //config: This utility is used to erase the whole MTD device. //applet:IF_FLASH_ERASEALL(APPLET(flash_eraseall, BB_DIR_USR_SBIN, BB_SUID_DROP)) +/* not NOEXEC: if flash operation stalls, use less memory in "hung" process */ //kbuild:lib-$(CONFIG_FLASH_ERASEALL) += flash_eraseall.o diff --git a/miscutils/flash_lock_unlock.c b/miscutils/flash_lock_unlock.c index 374eed5f6..6f2c049f4 100644 --- a/miscutils/flash_lock_unlock.c +++ b/miscutils/flash_lock_unlock.c @@ -20,6 +20,7 @@ // APPLET_ODDNAME:name main location suid_type help //applet:IF_FLASH_LOCK( APPLET_ODDNAME(flash_lock, flash_lock_unlock, BB_DIR_USR_SBIN, BB_SUID_DROP, flash_lock)) //applet:IF_FLASH_UNLOCK(APPLET_ODDNAME(flash_unlock, flash_lock_unlock, BB_DIR_USR_SBIN, BB_SUID_DROP, flash_unlock)) +/* not NOEXEC: if flash operation stalls, use less memory in "hung" process */ //kbuild:lib-$(CONFIG_FLASH_LOCK) += flash_lock_unlock.o //kbuild:lib-$(CONFIG_FLASH_UNLOCK) += flash_lock_unlock.o diff --git a/miscutils/flashcp.c b/miscutils/flashcp.c index d4ac62df4..c10b96ee8 100644 --- a/miscutils/flashcp.c +++ b/miscutils/flashcp.c @@ -14,6 +14,7 @@ //config: This utility is used to copy images into a MTD device. //applet:IF_FLASHCP(APPLET(flashcp, BB_DIR_USR_SBIN, BB_SUID_DROP)) +/* not NOEXEC: if flash operation stalls, use less memory in "hung" process */ //kbuild:lib-$(CONFIG_FLASHCP) += flashcp.o diff --git a/miscutils/ubi_tools.c b/miscutils/ubi_tools.c index 494718ccf..123551e94 100644 --- a/miscutils/ubi_tools.c +++ b/miscutils/ubi_tools.c @@ -52,6 +52,7 @@ //applet:IF_UBIRMVOL( APPLET_ODDNAME(ubirmvol, ubi_tools, BB_DIR_USR_SBIN, BB_SUID_DROP, ubirmvol)) //applet:IF_UBIRSVOL( APPLET_ODDNAME(ubirsvol, ubi_tools, BB_DIR_USR_SBIN, BB_SUID_DROP, ubirsvol)) //applet:IF_UBIUPDATEVOL(APPLET_ODDNAME(ubiupdatevol, ubi_tools, BB_DIR_USR_SBIN, BB_SUID_DROP, ubiupdatevol)) +/* not NOEXEC: if flash operation stalls, use less memory in "hung" process */ //kbuild:lib-$(CONFIG_UBIATTACH) += ubi_tools.o //kbuild:lib-$(CONFIG_UBIDETACH) += ubi_tools.o @@ -83,16 +84,16 @@ #define do_rsvol (ENABLE_UBIRSVOL && (UBI_APPLET_CNT == 1 || applet_name[4] == 's')) #define do_update (ENABLE_UBIUPDATEVOL && (UBI_APPLET_CNT == 1 || applet_name[4] == 'p')) -static unsigned get_num_from_file(const char *path, unsigned max, const char *errmsg) +static unsigned get_num_from_file(const char *path, unsigned max) { char buf[sizeof(long long)*3]; unsigned long long num; if (open_read_close(path, buf, sizeof(buf)) < 0) - bb_perror_msg_and_die(errmsg, path); + bb_perror_msg_and_die("can't open '%s'", path); /* It can be \n terminated, xatoull won't work well */ if (sscanf(buf, "%llu", &num) != 1 || num > max) - bb_error_msg_and_die(errmsg, path); + bb_error_msg_and_die("number in '%s' is malformed or too large", path); return num; } @@ -226,10 +227,10 @@ int ubi_tools_main(int argc UNUSED_PARAM, char **argv) p = path_sys_class_ubi_ubi + sprintf(path_sys_class_ubi_ubi, "%u/", num); strcpy(p, "avail_eraseblocks"); - leb_avail = get_num_from_file(path, UINT_MAX, "Can't get available eraseblocks from '%s'"); + leb_avail = get_num_from_file(path, UINT_MAX); strcpy(p, "eraseblock_size"); - leb_size = get_num_from_file(path, MAX_SANE_ERASEBLOCK, "Can't get eraseblock size from '%s'"); + leb_size = get_num_from_file(path, MAX_SANE_ERASEBLOCK); size_bytes = leb_avail * (unsigned long long)leb_size; //if (size_bytes <= 0) @@ -241,16 +242,19 @@ int ubi_tools_main(int argc UNUSED_PARAM, char **argv) if (!(opts & OPTION_N)) bb_error_msg_and_die("name not specified"); + /* the structure is memset(0) above */ mkvol_req.vol_id = vol_id; mkvol_req.vol_type = UBI_DYNAMIC_VOLUME; if ((opts & OPTION_t) && type[0] == 's') mkvol_req.vol_type = UBI_STATIC_VOLUME; mkvol_req.alignment = alignment; mkvol_req.bytes = size_bytes; /* signed int64_t */ - strncpy(mkvol_req.name, vol_name, UBI_MAX_VOLUME_NAME); - mkvol_req.name_len = strlen(vol_name); + /* strnlen avoids overflow of 16-bit field (paranoia) */ + mkvol_req.name_len = strnlen(vol_name, UBI_MAX_VOLUME_NAME+1); if (mkvol_req.name_len > UBI_MAX_VOLUME_NAME) bb_error_msg_and_die("volume name too long: '%s'", vol_name); + /* this is safe: .name[] is UBI_MAX_VOLUME_NAME+1 bytes */ + strcpy(mkvol_req.name, vol_name); xioctl(fd, UBI_IOCMKVOL, &mkvol_req); } else @@ -315,38 +319,49 @@ int ubi_tools_main(int argc UNUSED_PARAM, char **argv) else { unsigned ubinum, volnum; unsigned leb_size; - ssize_t len; - char *input_data; + char *buf; /* Assume that device is in normal format. */ /* Removes need for scanning sysfs tree as full libubi does. */ if (sscanf(ubi_ctrl, "/dev/ubi%u_%u", &ubinum, &volnum) != 2) - bb_error_msg_and_die("wrong format of UBI device name"); + bb_error_msg_and_die("UBI device name '%s' is not /dev/ubiN_M", ubi_ctrl); sprintf(path_sys_class_ubi_ubi, "%u_%u/usable_eb_size", ubinum, volnum); - leb_size = get_num_from_file(path, MAX_SANE_ERASEBLOCK, "Can't get usable eraseblock size from '%s'"); + leb_size = get_num_from_file(path, MAX_SANE_ERASEBLOCK); - if (!(opts & OPTION_t)) { - if (!*argv) - bb_show_usage(); + if (!*argv) + bb_show_usage(); + if (NOT_LONE_DASH(*argv)) /* mtd-utils supports "-" as stdin */ xmove_fd(xopen(*argv, O_RDONLY), STDIN_FILENO); - if (!(opts & OPTION_s)) { - struct stat st; - xfstat(STDIN_FILENO, &st, *argv); - size_bytes = st.st_size; - } + + if (!(opts & OPTION_s)) { + struct stat st; + xfstat(STDIN_FILENO, &st, *argv); + size_bytes = st.st_size; } bytes64 = size_bytes; /* this ioctl expects signed int64_t* parameter */ xioctl(fd, UBI_IOCVOLUP, &bytes64); - input_data = xmalloc(leb_size); - while ((len = full_read(STDIN_FILENO, input_data, leb_size)) > 0) { - xwrite(fd, input_data, len); + /* can't use bb_copyfd_exact_size(): copy in blocks of exactly leb_size */ + buf = xmalloc(leb_size); + while (size_bytes != 0) { + int len = full_read(STDIN_FILENO, buf, leb_size); + if (len <= 0) { + if (len < 0) + bb_perror_msg_and_die("read error from '%s'", *argv); + break; + } + if ((unsigned)len > size_bytes) { + /* for this case: "ubiupdatevol -s 1024000 $UBIDEV /dev/urandom" */ + len = size_bytes; + } + xwrite(fd, buf, len); + size_bytes -= len; } - if (len < 0) - bb_perror_msg_and_die("UBI volume update failed"); + if (ENABLE_FEATURE_CLEAN_UP) + free(buf); } } diff --git a/miscutils/ubirename.c b/miscutils/ubirename.c index 786c4b9fa..ecc8fe137 100644 --- a/miscutils/ubirename.c +++ b/miscutils/ubirename.c @@ -14,6 +14,7 @@ //config: Utility to rename UBI volumes //applet:IF_UBIRENAME(APPLET(ubirename, BB_DIR_USR_SBIN, BB_SUID_DROP)) +/* not NOEXEC: if flash operation stalls, use less memory in "hung" process */ //kbuild:lib-$(CONFIG_UBIRENAME) += ubirename.o @@ -80,9 +81,12 @@ int ubirename_main(int argc, char **argv) argv += 2; while (argv[0]) { rnvol->ents[n].vol_id = ubi_get_volid_by_name(ubi_devnum, argv[0]); - rnvol->ents[n].name_len = strlen(argv[1]); + + /* strnlen avoids overflow of 16-bit field (paranoia) */ + rnvol->ents[n].name_len = strnlen(argv[1], sizeof(rnvol->ents[n].name)); if (rnvol->ents[n].name_len >= sizeof(rnvol->ents[n].name)) bb_error_msg_and_die("new name '%s' is too long", argv[1]); + strcpy(rnvol->ents[n].name, argv[1]); n++; argv += 2;