[nbdkit PATCH] server: Don't assert on send if client hangs up early
by Eric Blake
libnbd's copy/copy-nbd-error.sh was triggering an assertion failure in
nbdkit:
$ nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M error-pread-rate=0.5 ] null:
...
nbdkit: pattern.2: debug: error-inject: pread count=262144 offset=4718592
nbdkit: pattern.2: debug: pattern: pread count=262144 offset=4718592
nbdkit: pattern.1: debug: error-inject: pread count=262144 offset=4456448
nbdkit: pattern.1: error: injecting EIO error into pread
nbdkit: pattern.1: debug: sending error reply: Input/output error
nbdkit: pattern.3: debug: error-inject: pread count=262144 offset=4980736
nbdkit: pattern.3: debug: pattern: pread count=262144 offset=4980736
nbdkit: pattern.4: error: write data: NBD_CMD_READ: Broken pipe
nbdkit: pattern.4: debug: exiting worker thread pattern.4
nbdkit: connections.c:402: raw_send_socket: Assertion `sock >= 0' failed.
When there are multiple queued requests, and the client hangs up
abruptly as soon as the first error response is seen (rather than
waiting until all other queued responses are flushed then sending
NBD_CMD_DISC), another thread that has a queued response ready to send
sees EPIPE (the client is no longer reading) and closes the socket,
then the third thread triggers the assertion failure because it not
expecting to attempt a send to a closed socket. Thus, my claim that
sock would always be non-negative after collecting data from the
plugin was wrong. The fix is to gracefully handle an abrupt client
disconnect, by not attempting to send on an already-closed socket.
For the nbdcopy example, --exit-with-parent means it's just an
annoyance (nbdcopy has already exited, and no further client will be
connecting); but for a longer-running nbdkit server accepting parallel
clients, it means any one client can trigger the SIGABRT by
intentionally queueing multiple NBD_CMD_READ then disconnecting early,
and thereby kill nbdkit and starve other clients. Whether it rises to
the level of CVE depends on whether you consider one client being able
to starve others a privilege escalation (if you are not using TLS,
there are other ways for a bad client to starve peers; if you are
using TLS, then the starved client has the same credentials as the
client that caused the SIGABRT so there is no privilege boundary
escalation).
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173054
Fixes: daef505e ("server: Give client EOF when we are done writing", v1.32.4)
---
server/connections.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/server/connections.c b/server/connections.c
index 4d776f2a..1b63eb73 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2022 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -400,7 +400,10 @@ raw_send_socket (const void *vbuf, size_t len, int flags)
ssize_t r;
int f = 0;
- assert (sock >= 0);
+ if (sock < 0) {
+ errno = EBADF;
+ return -1;
+ }
#ifdef MSG_MORE
if (flags & SEND_MORE)
f |= MSG_MORE;
--
2.39.2
2 years, 1 month
[PATCH 1/2] python: Avoid crash if callback parameters cannot be built
by Richard W.M. Jones
In the case that building the parameters to the Python event callback
fails, args was returned as NULL. We immediately tried to call
Py_INCREF on this which crashed. Returning NULL means the Python
function threw an exception, so print the exception and return (there
is no way to return an error here - the event is lost).
Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
python/handle.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/python/handle.c b/python/handle.c
index c8752baf47..f37e939e03 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
args = Py_BuildValue ("(Kis#O)",
(unsigned PY_LONG_LONG) event, event_handle,
buf, buf_len, py_array);
+ if (args == NULL) {
+ PyErr_PrintEx (0);
+ goto out;
+ }
+
Py_INCREF (args);
-
py_r = PyObject_CallObject (py_callback, args);
-
Py_DECREF (args);
-
if (py_r != NULL)
Py_DECREF (py_r);
else
/* Callback threw an exception: print it. */
PyErr_PrintEx (0);
+ out:
PyGILState_Release (py_save);
}
--
2.39.0
2 years, 1 month
[PATCH v2v v2 0/3] Use host-model
by Richard W.M. Jones
Version 1 was here:
https://listman.redhat.com/archives/libguestfs/2023-February/thread.html#...
I made a few changes in v2 but overall decided to keep the now unused
gcaps_arch_min_version capability. This doesn't preclude removing it
in future if we think it's never going to be useful.
I changed patch 1 so that to remove the long comment about how the
field is used, anticipating the later patches.
I changed patch 2 as Laszlo suggested.
I changed patch 3 as Laszlo suggested, but this also allows us to use
Option.default so it becomes a nice one-liner.
Rich.
2 years, 1 month
[PATCH v2v 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version
by Richard W.M. Jones
Some guests require not just a specific architecture, but cannot run
on qemu's default CPU model, eg. requiring x86_64-v2. Since we
anticipate future guests requiring higher versions, let's encode the
minimum architecture version instead of a simple boolean.
This patch essentially just remaps:
gcaps_default_cpu = true => gcaps_arch_min_version = 0
gcaps_default_cpu = false => gcaps_arch_min_version = 2
and updates the comments.
Updates: commit a50b975024ac5e46e107882e27fea498bf425685
---
lib/types.mli | 19 +++++++++++--------
convert/convert_linux.ml | 9 +++++----
convert/convert_windows.ml | 2 +-
lib/types.ml | 6 +++---
output/create_libvirt_xml.ml | 4 ++--
output/output_qemu.ml | 6 +++---
6 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/lib/types.mli b/lib/types.mli
index 24a93760cf..4a2bb5b371 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -263,24 +263,27 @@ type guestcaps = {
gcaps_machine : guestcaps_machine; (** Machine model. *)
gcaps_arch : string; (** Architecture that KVM must emulate. *)
- gcaps_virtio_1_0 : bool;
- (** The guest supports the virtio devices that it does at the virtio-1.0
- protocol level. *)
+ gcaps_arch_min_version : int;
+ (** Some guest OSes require not just a specific architecture, but a
+ minimum version. Notably RHEL >= 9 requires at least x86_64-v2.
- gcaps_default_cpu : bool;
- (** True iff the guest OS is capable of running on QEMU's default VCPU model
- (eg. "-cpu qemu64" with the Q35 and I440FX machine types).
+ If the guest is capable of running on QEMU's default VCPU model
+ for the architecture then this is set to [0].
This capability only matters for the QEMU and libvirt output modules,
- where, in case the capability is false *and* the source hypervisor does
+ where, in case the capability is false {b and} the source hypervisor does
not specify a VCPU model, we must manually present the guest OS with a
VCPU that looks as close as possible to a physical CPU. (In that case, we
- specify host-passthrough.)
+ specify host-model.)
The management applications targeted by the RHV and OpenStack output
modules have their own explicit VCPU defaults, overriding the QEMU default
model even in case the source hypervisor does not specify a VCPU model;
those modules ignore this capability therefore. Refer to RHBZ#2076013. *)
+
+ gcaps_virtio_1_0 : bool;
+ (** The guest supports the virtio devices that it does at the virtio-1.0
+ protocol level. *)
}
(** Guest capabilities after conversion. eg. Was virtio found or installed? *)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 460cbff0ed..d5c0f24dbb 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ =
* microarchitecture level, which the default QEMU VCPU model does not
* satisfy. Refer to RHBZ#2076013 RHBZ#2166619.
*)
- let default_cpu_suffices = family <> `RHEL_family ||
- inspect.i_arch <> "x86_64" ||
- inspect.i_major_version < 9 in
+ let arch_min_version =
+ if family <> `RHEL_family || inspect.i_arch <> "x86_64" ||
+ inspect.i_major_version < 9
+ then 0 else 2 in
(* Return guest capabilities from the convert () function. *)
let guestcaps = {
@@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ =
gcaps_virtio_socket = kernel.ki_supports_virtio_socket;
gcaps_machine = machine;
gcaps_arch = Utils.kvm_arch inspect.i_arch;
+ gcaps_arch_min_version = arch_min_version;
gcaps_virtio_1_0 = virtio_1_0;
- gcaps_default_cpu = default_cpu_suffices;
} in
guestcaps
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
index 8d3737995f..9d8d271d05 100644
--- a/convert/convert_windows.ml
+++ b/convert/convert_windows.ml
@@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ static_ips =
gcaps_machine = of_virtio_win_machine_type
virtio_win_installed.Inject_virtio_win.machine;
gcaps_arch = Utils.kvm_arch inspect.i_arch;
+ gcaps_arch_min_version = 0;
gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
- gcaps_default_cpu = true;
} in
guestcaps
diff --git a/lib/types.ml b/lib/types.ml
index 98f8bc6fa5..6c019ae130 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -397,8 +397,8 @@ type guestcaps = {
gcaps_virtio_socket : bool;
gcaps_machine : guestcaps_machine;
gcaps_arch : string;
+ gcaps_arch_min_version : int;
gcaps_virtio_1_0 : bool;
- gcaps_default_cpu : bool;
}
and guestcaps_block_type = Virtio_blk | IDE
and guestcaps_net_type = Virtio_net | E1000 | RTL8139
@@ -426,8 +426,8 @@ let string_of_guestcaps gcaps =
gcaps_virtio_socket = %b\n\
gcaps_machine = %s\n\
gcaps_arch = %s\n\
+ gcaps_arch_min_version = %d\n\
gcaps_virtio_1_0 = %b\n\
- gcaps_default_cpu = %b\n\
"
(string_of_block_type gcaps.gcaps_block_bus)
(string_of_net_type gcaps.gcaps_net_bus)
@@ -437,8 +437,8 @@ let string_of_guestcaps gcaps =
gcaps.gcaps_virtio_socket
(string_of_machine gcaps.gcaps_machine)
gcaps.gcaps_arch
+ gcaps.gcaps_arch_min_version
gcaps.gcaps_virtio_1_0
- gcaps.gcaps_default_cpu
type target_buses = {
target_virtio_blk_bus : target_bus_slot array;
diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
index 60977cf5bb..e9c6c8c150 100644
--- a/output/create_libvirt_xml.ml
+++ b/output/create_libvirt_xml.ml
@@ -185,14 +185,14 @@ let create_libvirt_xml ?pool source inspect
];
if source.s_cpu_model <> None ||
- not guestcaps.gcaps_default_cpu ||
+ guestcaps.gcaps_arch_min_version >= 1 ||
source.s_cpu_topology <> None then (
let cpu_attrs = ref []
and cpu = ref [] in
(match source.s_cpu_model with
| None ->
- if not guestcaps.gcaps_default_cpu then
+ if guestcaps.gcaps_arch_min_version >= 1 then
List.push_back cpu_attrs ("mode", "host-model");
| Some model ->
List.push_back cpu_attrs ("match", "minimum");
diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index b667e782ed..491906ebf9 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -175,9 +175,9 @@ module QEMU = struct
arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
- (match source.s_cpu_model, guestcaps.gcaps_default_cpu with
- | None, true -> ()
- | None, false -> arg "-cpu" "host"
+ (match source.s_cpu_model, guestcaps.gcaps_arch_min_version with
+ | None, 0 -> ()
+ | None, _ -> arg "-cpu" "host"
| Some model, _ -> arg "-cpu" model
);
--
2.39.0
2 years, 1 month
