From 54a53a006ed9c1fe027fd89045d6de1e9128d7f4 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sat, 30 Jul 2022 13:26:55 +0100 Subject: [PATCH 1/5] scsi-disk: fix overflow when block size is not a multiple of BDRV_SECTOR_SIZE In scsi_disk_emulate_write_same() the number of host sectors to transfer is calculated as (s->qdev.blocksize / BDRV_SECTOR_SIZE) which is then used to copy data in block size chunks to the iov buffer. Since the loop copying the data to the iov buffer uses a fixed increment of s->qdev.blocksize then using a block size that isn't a multiple of BDRV_SECTOR_SIZE introduces a rounding error in the iov buffer size calculation such that the iov buffer copy overflows the space allocated. Update the iov buffer copy for() loop so that it will use the smallest of either the current block size or the remaining transfer count to prevent the overflow. Signed-off-by: Mark Cave-Ayland Message-Id: <20220730122656.253448-2-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-disk.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index f5cdb9ad4b..3027ac3b1e 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1849,7 +1849,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) uint32_t nb_sectors = scsi_data_cdb_xfer(r->req.cmd.buf); WriteSameCBData *data; uint8_t *buf; - int i; + int i, l; /* Fail if PBDATA=1 or LBDATA=1 or ANCHOR=1. */ if (nb_sectors == 0 || (req->cmd.buf[1] & 0x16)) { @@ -1891,8 +1891,9 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) data->iov.iov_len); qemu_iovec_init_external(&data->qiov, &data->iov, 1); - for (i = 0; i < data->iov.iov_len; i += s->qdev.blocksize) { - memcpy(&buf[i], inbuf, s->qdev.blocksize); + for (i = 0; i < data->iov.iov_len; i += l) { + l = MIN(s->qdev.blocksize, data->iov.iov_len - i); + memcpy(&buf[i], inbuf, l); } scsi_req_ref(&r->req); From 55794c904df723109b228da28b5db778e0df3110 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sat, 30 Jul 2022 13:26:56 +0100 Subject: [PATCH 2/5] scsi-disk: ensure block size is non-zero and changes limited to bits 8-15 The existing code assumes that the block size can be generated from p[1] << 8 in multiple places which ignores the top and bottom 8 bits. If the block size is allowed to be set to an arbitrary value then this causes a mismatch between the value written by the guest in the block descriptor and the value subsequently read back using READ CAPACITY causing the guest to generate requests that can crash QEMU. For now restrict block size changes to bits 8-15 and also ignore requests to set the block size to 0 which causes the SCSI emulation to crash in at least one place with a divide by zero error. Fixes: 356c4c441e ("scsi-disk: allow MODE SELECT block descriptor to set the block size") Closes: https://gitlab.com/qemu-project/qemu/-/issues/1112 Signed-off-by: Mark Cave-Ayland Message-Id: <20220730122656.253448-3-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- hw/scsi/scsi-disk.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 3027ac3b1e..efee6739f9 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1591,7 +1591,7 @@ static void scsi_disk_emulate_mode_select(SCSIDiskReq *r, uint8_t *inbuf) int cmd = r->req.cmd.buf[0]; int len = r->req.cmd.xfer; int hdr_len = (cmd == MODE_SELECT ? 4 : 8); - int bd_len; + int bd_len, bs; int pass; if ((r->req.cmd.buf[1] & 0x11) != 0x10) { @@ -1617,9 +1617,19 @@ static void scsi_disk_emulate_mode_select(SCSIDiskReq *r, uint8_t *inbuf) } /* Allow changing the block size */ - if (bd_len && p[6] != (s->qdev.blocksize >> 8)) { - s->qdev.blocksize = p[6] << 8; - trace_scsi_disk_mode_select_set_blocksize(s->qdev.blocksize); + if (bd_len) { + bs = p[5] << 16 | p[6] << 8 | p[7]; + + /* + * Since the existing code only checks/updates bits 8-15 of the block + * size, restrict ourselves to the same requirement for now to ensure + * that a block size set by a block descriptor and then read back by + * a subsequent SCSI command will be the same + */ + if (bs && !(bs & ~0xff00) && bs != s->qdev.blocksize) { + s->qdev.blocksize = bs; + trace_scsi_disk_mode_select_set_blocksize(s->qdev.blocksize); + } } len -= bd_len; From e12f0685e8f6ee57a00d3f304023665aa0e02b8c Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 5 Aug 2022 12:01:51 +0200 Subject: [PATCH 3/5] vl: fix [memory] section with -readconfig The -M memory.* options do not have magic applied to them like the -m option, namely no "M" (for mebibytes) is tacked at the end of a suffixless value for "-M memory.size". This magic is performed by parse_memory_options, and we have to do it for both "-m" and the [memory] section of a config file. Storing [memory] sections directly to machine_opts_dict changed the meaning of [memory] size = "1024" in a -readconfig file from 1024MiB to 8KiB (1024 Bytes rounded up to 8KiB silently). To avoid this, the [memory] section has to be changed back to QemuOpts (combining [memory] and "-m" will work fine thanks to .merge_lists being true). Change parse_memory_options() so that, similar to the older function set_memory_options(), it operates after command line parsing is done; and also call it where set_memory_options() used to be. Note, the parsing code uses exit(1) instead of exit(EXIT_FAILURE) to match neighboring code. Reported-by: Markus Armbruster Reviewed-by: Markus Armbruster Fixes: ce9d03fb3f ("machine: add mem compound property", 2022-05-12) Signed-off-by: Paolo Bonzini --- softmmu/vl.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index aabd82e09a..45e919de9f 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1947,16 +1947,15 @@ static void qemu_resolve_machine_memdev(void) } } -static void parse_memory_options(const char *arg) +static void parse_memory_options(void) { - QemuOpts *opts; + QemuOpts *opts = qemu_find_opts_singleton("memory"); QDict *dict, *prop; const char *mem_str; + Location loc; - opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), arg, true); - if (!opts) { - exit(EXIT_FAILURE); - } + loc_push_none(&loc); + qemu_opts_loc_restore(opts); prop = qdict_new(); @@ -1987,6 +1986,7 @@ static void parse_memory_options(const char *arg) qdict_put(dict, "memory", prop); keyval_merge(machine_opts_dict, dict, &error_fatal); qobject_unref(dict); + loc_pop(&loc); } static void qemu_create_machine(QDict *qdict) @@ -2053,8 +2053,7 @@ static bool is_qemuopts_group(const char *group) if (g_str_equal(group, "object") || g_str_equal(group, "machine") || g_str_equal(group, "smp-opts") || - g_str_equal(group, "boot-opts") || - g_str_equal(group, "memory")) { + g_str_equal(group, "boot-opts")) { return false; } return true; @@ -2078,8 +2077,6 @@ static void qemu_record_config_group(const char *group, QDict *dict, machine_merge_property("smp", dict, &error_fatal); } else if (g_str_equal(group, "boot-opts")) { machine_merge_property("boot", dict, &error_fatal); - } else if (g_str_equal(group, "memory")) { - machine_merge_property("memory", dict, &error_fatal); } else { abort(); } @@ -2882,7 +2879,10 @@ void qemu_init(int argc, char **argv, char **envp) exit(0); break; case QEMU_OPTION_m: - parse_memory_options(optarg); + opts = qemu_opts_parse_noisily(qemu_find_opts("memory"), optarg, true); + if (opts == NULL) { + exit(1); + } break; #ifdef CONFIG_TPM case QEMU_OPTION_tpmdev: @@ -3515,6 +3515,9 @@ void qemu_init(int argc, char **argv, char **envp) configure_rtc(qemu_find_opts_singleton("rtc")); + /* Transfer QemuOpts options into machine options */ + parse_memory_options(); + qemu_create_machine(machine_opts_dict); suspend_mux_open(); From 69c05a23787e9f5cdddbf3688c2e309a92a20af4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 5 Aug 2022 19:15:39 +0200 Subject: [PATCH 4/5] vl: remove dead code in parse_memory_options() mem_str will never be an empty string, because qemu_opt_get_size() fails if it encounters one: $ ./qemu-system-x86_64 -m size= qemu-system-x86_64: -m size=: Parameter size expects a non-negative number below 2^64 Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta- and exabytes, respectively. Suggested-by: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Paolo Bonzini --- softmmu/vl.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index 45e919de9f..706bd7cff7 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1960,13 +1960,8 @@ static void parse_memory_options(void) prop = qdict_new(); if (qemu_opt_get_size(opts, "size", 0) != 0) { - mem_str = qemu_opt_get(opts, "size"); - if (!*mem_str) { - error_report("missing 'size' option value"); - exit(EXIT_FAILURE); - } - /* Fix up legacy suffix-less format */ + mem_str = qemu_opt_get(opts, "size"); if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) { g_autofree char *mib_str = g_strdup_printf("%sM", mem_str); qdict_put_str(prop, "size", mib_str); From f6a5f380627ab2af384bf2f2940d29386dea11ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 5 Aug 2022 12:55:29 +0100 Subject: [PATCH 5/5] tests/qtest: add scenario for -readconfig handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This test of -readconfig validates the last three regressions we have fixed with -readconfig: * Interpretation of memory size units as MiB not bytes * Allow use of [spice] * Allow use of [object] Signed-off-by: Daniel P. Berrangé Message-Id: <20220805115529.124544-2-berrange@redhat.com> Signed-off-by: Paolo Bonzini --- tests/qtest/meson.build | 1 + tests/qtest/readconfig-test.c | 195 ++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+) create mode 100644 tests/qtest/readconfig-test.c diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 3a474010e4..be4b30dea2 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -26,6 +26,7 @@ qtests_generic = [ 'qom-test', 'test-hmp', 'qos-test', + 'readconfig-test', ] if config_host.has_key('CONFIG_MODULES') qtests_generic += [ 'modules-test' ] diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c new file mode 100644 index 0000000000..2e604d7c2d --- /dev/null +++ b/tests/qtest/readconfig-test.c @@ -0,0 +1,195 @@ +/* + * Validate -readconfig + * + * Copyright (c) 2022 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "libqtest.h" +#include "qapi/error.h" +#include "qapi/qapi-visit-machine.h" +#include "qapi/qapi-visit-qom.h" +#include "qapi/qapi-visit-ui.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qmp/qlist.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qmp/qstring.h" +#include "qemu/units.h" + +static QTestState *qtest_init_with_config(const char *cfgdata) +{ + GError *error = NULL; + g_autofree char *args = NULL; + int cfgfd = -1; + g_autofree char *cfgpath = NULL; + QTestState *qts; + ssize_t ret; + + cfgfd = g_file_open_tmp("readconfig-test-XXXXXX", &cfgpath, &error); + g_assert_no_error(error); + g_assert_cmpint(cfgfd, >=, 0); + + ret = qemu_write_full(cfgfd, cfgdata, strlen(cfgdata)); + if (ret < 0) { + unlink(cfgpath); + } + g_assert_cmpint(ret, ==, strlen(cfgdata)); + + close(cfgfd); + + args = g_strdup_printf("-nodefaults -machine none -readconfig %s", cfgpath); + + qts = qtest_init(args); + + unlink(cfgpath); + + return qts; +} + +static void test_x86_memdev_resp(QObject *res) +{ + Visitor *v; + g_autoptr(MemdevList) memdevs = NULL; + Memdev *memdev; + + g_assert(res); + v = qobject_input_visitor_new(res); + visit_type_MemdevList(v, NULL, &memdevs, &error_abort); + + g_assert(memdevs); + g_assert(memdevs->value); + g_assert(!memdevs->next); + + memdev = memdevs->value; + g_assert_cmpstr(memdev->id, ==, "ram"); + g_assert_cmpint(memdev->size, ==, 200 * MiB); + + visit_free(v); +} + +static void test_x86_memdev(void) +{ + QDict *resp; + QTestState *qts; + const char *cfgdata = + "[memory]\n" + "size = \"200\""; + + qts = qtest_init_with_config(cfgdata); + /* Test valid command */ + resp = qtest_qmp(qts, "{ 'execute': 'query-memdev' }"); + test_x86_memdev_resp(qdict_get(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} + + +#ifdef CONFIG_SPICE +static void test_spice_resp(QObject *res) +{ + Visitor *v; + g_autoptr(SpiceInfo) spice = NULL; + + g_assert(res); + v = qobject_input_visitor_new(res); + visit_type_SpiceInfo(v, "spcie", &spice, &error_abort); + + g_assert(spice); + g_assert(spice->enabled); + + visit_free(v); +} + +static void test_spice(void) +{ + QDict *resp; + QTestState *qts; + const char *cfgdata = + "[spice]\n" + "disable-ticketing = \"on\"\n" + "unix = \"on\"\n"; + + qts = qtest_init_with_config(cfgdata); + /* Test valid command */ + resp = qtest_qmp(qts, "{ 'execute': 'query-spice' }"); + test_spice_resp(qdict_get(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} +#endif + +static void test_object_rng_resp(QObject *res) +{ + Visitor *v; + g_autoptr(ObjectPropertyInfoList) objs = NULL; + ObjectPropertyInfoList *tmp; + ObjectPropertyInfo *obj; + bool seen_rng = false; + + g_assert(res); + v = qobject_input_visitor_new(res); + visit_type_ObjectPropertyInfoList(v, NULL, &objs, &error_abort); + + g_assert(objs); + tmp = objs; + while (tmp) { + g_assert(tmp->value); + + obj = tmp->value; + if (g_str_equal(obj->name, "rng0") && + g_str_equal(obj->type, "child")) { + seen_rng = true; + } + + tmp = tmp->next; + } + + g_assert(seen_rng); + + visit_free(v); +} + +static void test_object_rng(void) +{ + QDict *resp; + QTestState *qts; + const char *cfgdata = + "[object]\n" + "qom-type = \"rng-builtin\"\n" + "id = \"rng0\"\n"; + + qts = qtest_init_with_config(cfgdata); + /* Test valid command */ + resp = qtest_qmp(qts, + "{ 'execute': 'qom-list'," + " 'arguments': {'path': '/objects' }}"); + test_object_rng_resp(qdict_get(resp, "return")); + qobject_unref(resp); + + qtest_quit(qts); +} + +int main(int argc, char *argv[]) +{ + const char *arch; + g_test_init(&argc, &argv, NULL); + + arch = qtest_get_arch(); + + if (g_str_equal(arch, "i386") || + g_str_equal(arch, "x86_64")) { + qtest_add_func("readconfig/x86/memdev", test_x86_memdev); + } +#ifdef CONFIG_SPICE + qtest_add_func("readconfig/spice", test_spice); +#endif + + qtest_add_func("readconfig/object-rng", test_object_rng); + + return g_test_run(); +}