diff --git a/module/nvpair/nvpair.c b/module/nvpair/nvpair.c index 07ac102145e2..52678bb2badb 100644 --- a/module/nvpair/nvpair.c +++ b/module/nvpair/nvpair.c @@ -135,7 +135,8 @@ #define NVP_SIZE_CALC(name_len, data_len) \ (NV_ALIGN((sizeof (nvpair_t)) + name_len) + NV_ALIGN(data_len)) -static int i_get_value_size(data_type_t type, const void *data, uint_t nelem); +static int i_get_value_size(data_type_t type, const void *data, uint_t nelem, + size_t max_size); static int nvlist_add_common(nvlist_t *nvl, const char *name, data_type_t type, uint_t nelem, const void *data); @@ -810,8 +811,10 @@ i_validate_nvpair(nvpair_t *nvp) * verify nvp_type, nvp_value_elem, and also possibly * verify string values and get the value size. */ - size2 = i_get_value_size(type, NVP_VALUE(nvp), NVP_NELEM(nvp)); size1 = nvp->nvp_size - NVP_VALOFF(nvp); + size2 = i_get_value_size(type, NVP_VALUE(nvp), NVP_NELEM(nvp), + size1); + if (size2 < 0 || size1 != NV_ALIGN(size2)) return (EFAULT); @@ -1002,12 +1005,21 @@ nvlist_remove_nvpair(nvlist_t *nvl, nvpair_t *nvp) * DATA_TYPE_STRING and * DATA_TYPE_STRING_ARRAY * Is data == NULL then the size of the string(s) is excluded. + * + * If 'max_size' is non-zero, then don't look beyond 'max_size' number of + * bytes when calculating a value size. Note that 'max_size' should include + * the NULL terminator byte when calculating string size. If 'max_size' is 0, + * it is ignored. */ static int -i_get_value_size(data_type_t type, const void *data, uint_t nelem) +i_get_value_size(data_type_t type, const void *data, uint_t nelem, + size_t max_size) { uint64_t value_sz; + if (max_size == 0) + max_size = INT32_MAX; + if (i_validate_type_nelem(type, nelem) != 0) return (-1); @@ -1052,10 +1064,15 @@ i_get_value_size(data_type_t type, const void *data, uint_t nelem) break; #endif case DATA_TYPE_STRING: - if (data == NULL) + if (data == NULL) { value_sz = 0; - else - value_sz = strlen(data) + 1; + } else { + value_sz = strnlen(data, max_size); + if (value_sz >= max_size) { + return (-1); /* string not terminated */ + } + value_sz += 1; + } break; case DATA_TYPE_BOOLEAN_ARRAY: value_sz = (uint64_t)nelem * sizeof (boolean_t); @@ -1089,16 +1106,23 @@ i_get_value_size(data_type_t type, const void *data, uint_t nelem) break; case DATA_TYPE_STRING_ARRAY: value_sz = (uint64_t)nelem * sizeof (uint64_t); - if (data != NULL) { char *const *strs = data; uint_t i; + size_t newsize; /* no alignment requirement for strings */ for (i = 0; i < nelem; i++) { if (strs[i] == NULL) return (-1); - value_sz += strlen(strs[i]) + 1; + + newsize = strnlen(strs[i], max_size); + + if (newsize == max_size) + return (-1); /* not terminated */ + + value_sz += newsize + 1; /* +1 for NULL */ + max_size -= newsize + 1; } } break; @@ -1163,7 +1187,7 @@ nvlist_add_common(nvlist_t *nvl, const char *name, * In case of data types DATA_TYPE_STRING and DATA_TYPE_STRING_ARRAY * is the size of the string(s) included. */ - if ((value_sz = i_get_value_size(type, data, nelem)) < 0) + if ((value_sz = i_get_value_size(type, data, nelem, 0)) < 0) return (EINVAL); if (i_validate_nvpair_value(type, nelem, data) != 0) @@ -1588,7 +1612,7 @@ nvpair_value_common(const nvpair_t *nvp, data_type_t type, uint_t *nelem, #endif if (data == NULL) return (EINVAL); - if ((value_sz = i_get_value_size(type, NULL, 1)) < 0) + if ((value_sz = i_get_value_size(type, NULL, 1, 0)) < 0) return (EINVAL); memcpy(data, NVP_VALUE(nvp), (size_t)value_sz); if (nelem != NULL) @@ -3019,7 +3043,8 @@ nvs_native_nvp_op(nvstream_t *nvs, nvpair_t *nvp) * In case of data types DATA_TYPE_STRING and DATA_TYPE_STRING_ARRAY * is the size of the string(s) excluded. */ - if ((value_sz = i_get_value_size(type, NULL, NVP_NELEM(nvp))) < 0) + if ((value_sz = i_get_value_size(type, NULL, NVP_NELEM(nvp), + NVP_SIZE(nvp))) < 0) return (EFAULT); if (NVP_SIZE_CALC(nvp->nvp_name_sz, value_sz) > nvp->nvp_size) @@ -3333,7 +3358,7 @@ nvs_xdr_nvp_op(nvstream_t *nvs, nvpair_t *nvp) * In case of data types DATA_TYPE_STRING and DATA_TYPE_STRING_ARRAY * is the size of the string(s) excluded. */ - if ((value_sz = i_get_value_size(type, NULL, nelem)) < 0) + if ((value_sz = i_get_value_size(type, NULL, nelem, NVP_SIZE(nvp)) < 0)) return (EFAULT); /* if there is no data to extract then return */ diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index d31aa80641ce..7013fbfb64fb 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -4126,7 +4126,6 @@ static int zfs_ioc_log_history(const char *unused, nvlist_t *innvl, nvlist_t *outnvl) { (void) unused, (void) outnvl; - const char *message; char *poolname; spa_t *spa; int error; @@ -4147,7 +4146,7 @@ zfs_ioc_log_history(const char *unused, nvlist_t *innvl, nvlist_t *outnvl) if (error != 0) return (error); - message = fnvlist_lookup_string(innvl, "message"); + const char *message = fnvlist_lookup_string(innvl, "message"); if (spa_version(spa) < SPA_VERSION_ZPOOL_HISTORY) { spa_close(spa, FTAG); diff --git a/tests/zfs-tests/cmd/libzfs_input_check.c b/tests/zfs-tests/cmd/libzfs_input_check.c index 4ef249bbd4a1..8f7e36d9efa7 100644 --- a/tests/zfs-tests/cmd/libzfs_input_check.c +++ b/tests/zfs-tests/cmd/libzfs_input_check.c @@ -85,7 +85,6 @@ static const zfs_ioc_t ioc_skip[] = { ZFS_IOC_DSOBJ_TO_DSNAME, ZFS_IOC_OBJ_TO_PATH, ZFS_IOC_POOL_SET_PROPS, - ZFS_IOC_POOL_GET_PROPS, ZFS_IOC_SET_FSACL, ZFS_IOC_GET_FSACL, ZFS_IOC_SHARE, @@ -125,11 +124,136 @@ static const zfs_ioc_t ioc_skip[] = { lzc_ioctl_test(ioc, name, req, opt, err, wild); \ } while (0) +#define IOC_INPUT_TEST_INJECT(ioc, name, innvl) \ + do { \ + active_test = __func__ + 5; \ + lzc_ioctl_run_impl(ioc, name, innvl, 0, B_TRUE); \ + } while (0) + +/* + * Given a zfs_cmd_t containing an already packed nvlist in zc->zc_nvlist_src, + * and its original innvl, look in innvl for the last string nvpair, or last + * string array nvpair, and remove the string terminator. The idea is to + * corrupt the nvlist string value so that anyone doing a strlen() on it will + * read past the end of the packed nvlist buffer and trigger a crash. + */ +static void +do_bad_string(zfs_cmd_t *zc, nvlist_t *innvl) +{ + nvpair_t *elem = NULL; + nvpair_t *lastseen = NULL; + const char *str = NULL; + const char **arr; + uint_t n; + char *off; + char *packed; + uint64_t size, off_size; + + while ((elem = nvlist_next_nvpair(innvl, elem)) != NULL) { + if ((nvpair_type(elem) == DATA_TYPE_STRING) || + (nvpair_type(elem) == DATA_TYPE_STRING_ARRAY)) + lastseen = elem; + } + + if (lastseen == NULL) + return; /* No strings */ + + /* + * Lookup either the last string, or the last string in the last + * string array in the nvlist. We will use this to corrupt from the + * string to the end of the nvlist buffer. Any attempts to strlen this + * string should run pass the end of the packed buffer. + */ + if (nvpair_value_string(lastseen, &str) != 0) { + if (nvpair_value_string_array(lastseen, &arr, &n) == 0) + str = arr[n-1]; + } + + /* + * We now have the last string. Corrupt everything from the NULL + * terminator byte for the last string to the end of the packed nvlist + * buffer. + */ + packed = (char *)zc->zc_nvlist_src; + size = zc->zc_nvlist_src_size; + + off = memmem(packed, size, str, strlen(str)); + off_size = strlen(str); + + memset(&off[off_size - 1], '!', (packed + size) - + (&off[off_size - 1])); + +} + +/* + * For each byte in the packed nvlist list in zc, corrupt a single byte, then + * try doing the ioctl. This tests how well the kernel handles fuzzed nvlists. + * + * NOTE - make sure you are doing this with a "safe" ioctl! You don't want to + * run this on an ioctl that can potentially corrupt data (like a zpool create). + */ +static void +do_fuzz(int zfs_fd, zfs_ioc_t ioc, zfs_cmd_t *zc) +{ + uint64_t size; + uint64_t i; + unsigned char old = 0; + unsigned char *pos; + zfs_cmd_t orig_zc = *zc; + + pos = (unsigned char *) zc->zc_nvlist_src; + size = zc->zc_nvlist_src_size; + + /* + * Fuzz each byte in the packed nvlist, one byte at a time, and do the + * ioctl. If the kernel doesn't crash, then the test passed. + */ + for (i = 0; i < size; i++) { + /* Restore the previously corrupted byte */ + if (i > 0) + pos[i-1] = old; + + old = pos[i]; + + /* Corrupt the new byte */ + pos[i]++; + + /* + * Do the ioctl and ignore the return code. We just want to + * see if the kernel panics. + */ + lzc_ioctl_fd(zfs_fd, ioc, zc); + + /* + * Restore 'zc' with original fields since the ioctl may + * have modified them. + */ + *zc = orig_zc; + } + /* Restore last byte */ + if (i > 0) + pos[i - 1] = old; + + /* + * Try fuzzing the packed nvlist size field. Test it with one byte + * bigger and one byte smaller than the current value. + */ + zc->zc_nvlist_src_size--; + lzc_ioctl_fd(zfs_fd, ioc, zc); + + zc->zc_nvlist_src_size += 2; + lzc_ioctl_fd(zfs_fd, ioc, zc); + + /* Restore to normal */ + zc->zc_nvlist_src_size -= 1; +} + /* * run a zfs ioctl command, verify expected results and log failures */ static void -lzc_ioctl_run(zfs_ioc_t ioc, const char *name, nvlist_t *innvl, int expected) +lzc_ioctl_run_impl(zfs_ioc_t ioc, const char *name, nvlist_t *innvl, + int expected, boolean_t do_corrupt) { zfs_cmd_t zc = {"\0"}; char *packed = NULL; @@ -160,10 +284,30 @@ lzc_ioctl_run(zfs_ioc_t ioc, const char *name, nvlist_t *innvl, int expected) zc.zc_nvlist_dst_size = MAX(size * 2, 128 * 1024); zc.zc_nvlist_dst = (uint64_t)(uintptr_t)malloc(zc.zc_nvlist_dst_size); + if (do_corrupt) { + /* + * Try changing bytes in the packed nvlist to see if it will + * panic the kernel when you do the ioctl. + */ + do_fuzz(zfs_fd, ioc, &zc); + + /* + * Corrupt the last string in the packed nvlist so it has no + * NULL terminator. + */ + do_bad_string(&zc, innvl); + + } + if (lzc_ioctl_fd(zfs_fd, ioc, &zc) != 0) error = errno; - if (error != expected) { + /* + * If we're corrupting the nvlist we don't care about the specific + * error code that gets returned, as it could be one of many. We only + * care if it panics the kernel. + */ + if (!do_corrupt && error != expected) { unexpected_failures = B_TRUE; (void) fprintf(stderr, "%s: Unexpected result with %s, " "error %d (expecting %d)\n", @@ -174,6 +318,12 @@ lzc_ioctl_run(zfs_ioc_t ioc, const char *name, nvlist_t *innvl, int expected) free((void *)(uintptr_t)zc.zc_nvlist_dst); } +static void +lzc_ioctl_run(zfs_ioc_t ioc, const char *name, nvlist_t *innvl, int expected) +{ + return (lzc_ioctl_run_impl(ioc, name, innvl, expected, B_FALSE)); +} + /* * Test each ioc for the following ioctl input errors: * ZFS_ERR_IOC_ARG_UNAVAIL an input argument is not supported by kernel @@ -310,6 +460,7 @@ test_log_history(const char *pool) fnvlist_add_string(required, "message", "input check"); IOC_INPUT_TEST(ZFS_IOC_LOG_HISTORY, pool, required, NULL, 0); + IOC_INPUT_TEST_INJECT(ZFS_IOC_LOG_HISTORY, pool, required); nvlist_free(required); } @@ -791,6 +942,20 @@ test_set_bootenv(const char *pool) nvlist_free(required); } +static void +test_zpool_get(const char *pool) +{ + const char *strs[] = {ZPOOL_DEDUPCACHED_PROP_NAME}; + nvlist_t *optional = fnvlist_alloc(); + + fnvlist_add_string_array(optional, ZPOOL_GET_PROPS_NAMES, strs, 1); + + IOC_INPUT_TEST(ZFS_IOC_POOL_GET_PROPS, pool, NULL, optional, 0); + IOC_INPUT_TEST_INJECT(ZFS_IOC_POOL_GET_PROPS, pool, optional); + + nvlist_free(optional); +} + static void zfs_ioc_input_tests(const char *pool) { @@ -885,6 +1050,7 @@ zfs_ioc_input_tests(const char *pool) test_scrub(pool); + test_zpool_get(pool); /* * cleanup */