From ee8c13b81474e002db083e9692b11c0e106a9c7f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 2 Jul 2018 14:21:58 +0200 Subject: [PATCH 1/8] pr-helper: avoid error on PR IN command with zero request size After reading a PR IN command with zero request size in prh_read_request, the resp->result field will be uninitialized and the resp.sz field will be also uninitialized when returning to prh_co_entry. If resp->result == GOOD (from a previous successful reply or just luck), then the assert in prh_write_response might not be triggered and uninitialized response will be sent. The fix is to remove the whole handling of sz == 0 in prh_co_entry. Those errors apply only to PR OUT commands and it's perfectly okay to catch them later in do_pr_out and multipath_pr_out; the check for too-short parameters in fact doesn't apply in the easy SG_IO case, as it can be left to the target firmware even. The result is that prh_read_request does not fail requests anymore and prh_co_entry becomes simpler. Reported-by: Dima Stepanov Signed-off-by: Paolo Bonzini --- scsi/qemu-pr-helper.c | 63 +++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 33 deletions(-) diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index 0218d65bbf..c89a446a45 100644 --- a/scsi/qemu-pr-helper.c +++ b/scsi/qemu-pr-helper.c @@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, char transportids[PR_HELPER_DATA_SIZE]; int r; + if (sz < PR_OUT_FIXED_PARAM_SIZE) { + /* Illegal request, Parameter list length error. This isn't fatal; + * we have read the data, send an error without closing the socket. + */ + scsi_build_sense(sense, SENSE_CODE(INVALID_PARAM_LEN)); + return CHECK_CONDITION; + } + switch (rq_servact) { case MPATH_PROUT_REG_SA: case MPATH_PROUT_RES_SA: @@ -574,6 +582,12 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, const uint8_t *param, int sz) { int resp_sz; + + if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { + scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); + return CHECK_CONDITION; + } + #ifdef CONFIG_MPATH if (is_mpath(fd)) { return multipath_pr_out(fd, cdb, sense, param, sz); @@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient *client, errp) < 0) { goto out_close; } - if ((fcntl(client->fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { - scsi_build_sense(resp->sense, SENSE_CODE(INVALID_OPCODE)); - sz = 0; - } else if (sz < PR_OUT_FIXED_PARAM_SIZE) { - /* Illegal request, Parameter list length error. This isn't fatal; - * we have read the data, send an error without closing the socket. - */ - scsi_build_sense(resp->sense, SENSE_CODE(INVALID_PARAM_LEN)); - sz = 0; - } - if (sz == 0) { - resp->result = CHECK_CONDITION; - close(client->fd); - client->fd = -1; - } } req->fd = client->fd; @@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque) break; } - if (sz > 0) { - num_active_sockets++; - if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { - r = do_pr_out(req.fd, req.cdb, resp.sense, - client->data, sz); - resp.sz = 0; - } else { - resp.sz = sizeof(client->data); - r = do_pr_in(req.fd, req.cdb, resp.sense, - client->data, &resp.sz); - resp.sz = MIN(resp.sz, sz); - } - num_active_sockets--; - close(req.fd); - if (r == -1) { - break; - } - resp.result = r; + num_active_sockets++; + if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { + r = do_pr_out(req.fd, req.cdb, resp.sense, + client->data, sz); + resp.sz = 0; + } else { + resp.sz = sizeof(client->data); + r = do_pr_in(req.fd, req.cdb, resp.sense, + client->data, &resp.sz); + resp.sz = MIN(resp.sz, sz); } + num_active_sockets--; + close(req.fd); + if (r == -1) { + break; + } + resp.result = r; if (prh_write_response(client, &req, &resp, &local_err) < 0) { break; From 2729d79d4993099782002c9a218de1fc12c32c69 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Tue, 3 Jul 2018 11:51:14 +0200 Subject: [PATCH 2/8] pr-helper: Rework socket path handling When reviewing Paolo's pr-helper patches I've noticed couple of problems: 1) socket_path needs to be calculated at two different places (one for printing out help, the other if socket activation is NOT used), 2) even though the default socket_path is allocated in compute_default_paths() it is the only default path the function handles. For instance, pidfile is allocated outside of this function. And yet again, at different places than 1) Signed-off-by: Michal Privoznik Message-Id: Signed-off-by: Paolo Bonzini --- scsi/qemu-pr-helper.c | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index c89a446a45..1528a712a0 100644 --- a/scsi/qemu-pr-helper.c +++ b/scsi/qemu-pr-helper.c @@ -76,14 +76,12 @@ static int gid = -1; static void compute_default_paths(void) { - if (!socket_path) { - socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock"); - } + socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock"); + pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid"); } static void usage(const char *name) { - compute_default_paths(); (printf) ( "Usage: %s [OPTIONS] FILE\n" "Persistent Reservation helper program for QEMU\n" @@ -841,19 +839,6 @@ static gboolean accept_client(QIOChannel *ioc, GIOCondition cond, gpointer opaqu return TRUE; } - -/* - * Check socket parameters compatibility when socket activation is used. - */ -static const char *socket_activation_validate_opts(void) -{ - if (socket_path != NULL) { - return "Unix socket can't be set when using socket activation"; - } - - return NULL; -} - static void termsig_handler(int signum) { atomic_cmpxchg(&state, RUNNING, TERMINATE); @@ -927,6 +912,7 @@ int main(int argc, char **argv) char *trace_file = NULL; bool daemonize = false; bool pidfile_specified = false; + bool socket_path_specified = false; unsigned socket_activation; struct sigaction sa_sigterm; @@ -943,12 +929,14 @@ int main(int argc, char **argv) qemu_add_opts(&qemu_trace_opts); qemu_init_exec_dir(argv[0]); - pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid"); + compute_default_paths(); while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) { switch (ch) { case 'k': - socket_path = optarg; + g_free(socket_path); + socket_path = g_strdup(optarg); + socket_path_specified = true; if (socket_path[0] != '/') { error_report("socket path must be absolute"); exit(EXIT_FAILURE); @@ -1039,10 +1027,9 @@ int main(int argc, char **argv) socket_activation = check_socket_activation(); if (socket_activation == 0) { SocketAddress saddr; - compute_default_paths(); saddr = (SocketAddress){ .type = SOCKET_ADDRESS_TYPE_UNIX, - .u.q_unix.path = g_strdup(socket_path) + .u.q_unix.path = socket_path, }; server_ioc = qio_channel_socket_new(); if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) < 0) { @@ -1050,12 +1037,10 @@ int main(int argc, char **argv) error_report_err(local_err); return 1; } - g_free(saddr.u.q_unix.path); } else { /* Using socket activation - check user didn't use -p etc. */ - const char *err_msg = socket_activation_validate_opts(); - if (err_msg != NULL) { - error_report("%s", err_msg); + if (socket_path_specified) { + error_report("Unix socket can't be set when using socket activation"); exit(EXIT_FAILURE); } @@ -1072,7 +1057,6 @@ int main(int argc, char **argv) error_get_pretty(local_err)); exit(EXIT_FAILURE); } - socket_path = NULL; } if (qemu_init_main_loop(&local_err)) { From 70da30483e78b501ff4b3a090f26c08abd0f7a7f Mon Sep 17 00:00:00 2001 From: Julia Suvorova Date: Mon, 2 Jul 2018 09:52:37 +0300 Subject: [PATCH 3/8] qtest: Use cpu address space instead of system memory Some devices (like nvic in armv7m) are not accessable through address_space_memory, therefore can not be tested with qtest. Signed-off-by: Julia Suvorova Message-Id: <20180702065237.27899-1-jusual@mail.ru> Signed-off-by: Paolo Bonzini --- qtest.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/qtest.c b/qtest.c index cbbfb71114..69b9e9962b 100644 --- a/qtest.c +++ b/qtest.c @@ -387,19 +387,23 @@ static void qtest_process_command(CharBackend *chr, gchar **words) if (words[0][5] == 'b') { uint8_t data = value; - cpu_physical_memory_write(addr, &data, 1); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + &data, 1, true); } else if (words[0][5] == 'w') { uint16_t data = value; tswap16s(&data); - cpu_physical_memory_write(addr, &data, 2); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &data, 2, true); } else if (words[0][5] == 'l') { uint32_t data = value; tswap32s(&data); - cpu_physical_memory_write(addr, &data, 4); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &data, 4, true); } else if (words[0][5] == 'q') { uint64_t data = value; tswap64s(&data); - cpu_physical_memory_write(addr, &data, 8); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &data, 8, true); } qtest_send_prefix(chr); qtest_send(chr, "OK\n"); @@ -417,18 +421,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words) if (words[0][4] == 'b') { uint8_t data; - cpu_physical_memory_read(addr, &data, 1); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + &data, 1, false); value = data; } else if (words[0][4] == 'w') { uint16_t data; - cpu_physical_memory_read(addr, &data, 2); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &data, 2, false); value = tswap16(data); } else if (words[0][4] == 'l') { uint32_t data; - cpu_physical_memory_read(addr, &data, 4); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &data, 4, false); value = tswap32(data); } else if (words[0][4] == 'q') { - cpu_physical_memory_read(addr, &value, 8); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + (uint8_t *) &value, 8, false); tswap64s(&value); } qtest_send_prefix(chr); @@ -448,7 +456,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) g_assert(len); data = g_malloc(len); - cpu_physical_memory_read(addr, data, len); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + data, len, false); enc = g_malloc(2 * len + 1); for (i = 0; i < len; i++) { @@ -473,7 +482,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) g_assert(ret == 0); data = g_malloc(len); - cpu_physical_memory_read(addr, data, len); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + data, len, false); b64_data = g_base64_encode(data, len); qtest_send_prefix(chr); qtest_sendf(chr, "OK %s\n", b64_data); @@ -507,7 +517,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) data[i] = 0; } } - cpu_physical_memory_write(addr, data, len); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + data, len, true); g_free(data); qtest_send_prefix(chr); @@ -529,7 +540,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) if (len) { data = g_malloc(len); memset(data, pattern, len); - cpu_physical_memory_write(addr, data, len); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + data, len, true); g_free(data); } @@ -562,7 +574,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words) out_len = MIN(out_len, len); } - cpu_physical_memory_write(addr, data, out_len); + address_space_rw(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, + data, len, true); qtest_send_prefix(chr); qtest_send(chr, "OK\n"); From 02693cc4f4aacf82cb73559dfa9af76ce4b13d11 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 2 Jul 2018 18:56:06 +0200 Subject: [PATCH 4/8] i386: fix '-cpu ?' output for host cpu type Since commit d6dcc5583e7, '-cpu ?' shows the description of the X86_CPU_TYPE_NAME("max") for the host CPU model: Enables all features supported by the accelerator in the current host instead of the expected: KVM processor with all supported host features or HVF processor with all supported host features This is caused by the early use of kvm_enabled() and hvf_enabled() in a class_init function. Since the accelerator isn't configured yet, both helpers return false unconditionally. A QEMU binary will only be compiled with one of these accelerators, not both. The appropriate description can thus be decided at build time. Signed-off-by: Greg Kurz Message-Id: <153055056654.212317.4697363278304826913.stgit@bahia.lan> Signed-off-by: Paolo Bonzini --- target/i386/cpu.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b0b87c3d81..e0e2f2eea1 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -2836,13 +2836,13 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data) xcc->host_cpuid_required = true; xcc->ordering = 8; - if (kvm_enabled()) { - xcc->model_description = - "KVM processor with all supported host features "; - } else if (hvf_enabled()) { - xcc->model_description = - "HVF processor with all supported host features "; - } +#if defined(CONFIG_KVM) + xcc->model_description = + "KVM processor with all supported host features "; +#elif defined(CONFIG_HVF) + xcc->model_description = + "HVF processor with all supported host features "; +#endif } static const TypeInfo host_x86_cpu_type_info = { From 81e34930ce2b605ce9dbb54a7b846379cbb1b811 Mon Sep 17 00:00:00 2001 From: "xinhua.Cao" Date: Wed, 4 Jul 2018 11:36:42 +0800 Subject: [PATCH 5/8] qemu-char: check errno together with ret < 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the tcp_chr_write function, we checked errno, but errno was not reset before a read or write operation. Therefore, this check of errno's actions is often incorrect after EAGAIN has occurred. we need check errno together with ret < 0. Signed-off-by: xinhua.Cao Message-Id: <20180704033642.15996-1-caoxinhua@huawei.com> Reviewed-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé Fixes: 9fc53a10f81d3a9027b23fa810147d21be29e614 Signed-off-by: Paolo Bonzini --- chardev/char-socket.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 17519ec589..efbad6ee7c 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len) s->write_msgfds, s->write_msgfds_num); - /* free the written msgfds in any cases other than errno==EAGAIN */ - if (EAGAIN != errno && s->write_msgfds_num) { + /* free the written msgfds in any cases + * other than ret < 0 && errno == EAGAIN + */ + if (!(ret < 0 && EAGAIN == errno) + && s->write_msgfds_num) { g_free(s->write_msgfds); s->write_msgfds = 0; s->write_msgfds_num = 0; From ea3d77c889cfa8c450da8a716c2bfd6aaea0adb2 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 4 Jul 2018 13:58:20 +0200 Subject: [PATCH 6/8] pr-manager-helper: fix memory leak on event Reported by Coverity. Signed-off-by: Paolo Bonzini --- scsi/pr-manager-helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c index 519a296905..3027dde60d 100644 --- a/scsi/pr-manager-helper.c +++ b/scsi/pr-manager-helper.c @@ -46,6 +46,7 @@ static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr) if (id) { qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc, &error_abort); + g_free(id); } } From 960a479f7f94bb615991d41b8c5ff4e3c7d0088d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 4 Jul 2018 14:03:10 +0200 Subject: [PATCH 7/8] ioapic: remove useless lower bounds check The vector cannot be negative. Coverity now reports this because it sees an array access before the check, in ioapic_stat_update_irq. Reviewed-by: Peter Xu Signed-off-by: Paolo Bonzini --- hw/intc/ioapic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index b3937807c2..b6896ac4ce 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -152,7 +152,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level) if (vector == 0) { vector = 2; } - if (vector >= 0 && vector < IOAPIC_NUM_PINS) { + if (vector < IOAPIC_NUM_PINS) { uint32_t mask = 1 << vector; uint64_t entry = s->ioredtbl[vector]; From e20122ff0faf07cb701d35e39e106d1783c07725 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 4 Jul 2018 18:05:43 +0200 Subject: [PATCH 8/8] checkpatch: handle token pasting better The mechanism to find possible type tokens can sometimes be confused and go into an infinite loop. This happens for example in QEMU for a line that looks like uint## BITS ##_t S = _S, T = _T; \ uint## BITS ##_t as, at, xs, xt, xd; \ Because the token pasting operator does not have a space before _t, it does not match $notPermitted. However, (?x) is turned on in the regular expression for modifiers, and thus ##_t matches the empty string. As a result, annotate_values goes in an infinite loop. The solution is simply to remove token pasting operators from the string before looking for modifiers. In the example above, the string uintBITS_t will be evaluated as a candidate modifier. This is not optimal, but it works as long as people do not write things like a##s##m, and it fits nicely into sub possible. For a similar reason, \# should be rejected always, even if it is not at end of line or followed by whitespace. The same patch was sent to the Linux kernel mailing list. Reported-by: Aleksandar Markovic Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 223681bfd0..42e1c50dd8 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1132,11 +1132,10 @@ sub possible { case| else| asm|__asm__| - do| - \#| - \#\# + do )(?:\s|$)| - ^(?:typedef|struct|enum)\b + ^(?:typedef|struct|enum)\b| + ^\# )}x; warn "CHECK<$possible> ($line)\n" if ($dbg_possible > 2); if ($possible !~ $notPermitted) { @@ -1146,7 +1145,7 @@ sub possible { if ($possible =~ /^\s*$/) { } elsif ($possible =~ /\s/) { - $possible =~ s/\s*$Type\s*//g; + $possible =~ s/\s*(?:$Type|\#\#)\s*//g; for my $modifier (split(' ', $possible)) { if ($modifier !~ $notPermitted) { warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);