[PATCH 0/2] Drivers: hv: vmbus: Fix the error paths of channel setup

[PATCH 0/2] Drivers: hv: vmbus: Fix the error paths of channel setup

[Yuqiang Sun]

When setting up a channel fails, vmbus_add_channel_work() and
vmbus_device_register() do not agree on who cleans up:

- vmbus_add_channel_kobj() drops the channel's last reference when it
  fails, and its callers then use the channel and drop it again (patch 1;
  this is what our static analysis reported, for the sub-channel path).
  Paths that fail before it put a kobject that was never initialized.

- When vmbus_device_register() fails, the device's release calls
  hv_process_channel_removal() on a channel that was not rescinded, which
  hits BUG_ON(!channel->rescind); vmbus_add_channel_work() would then
  clean the channel up a second time (patch 2).

Testing: QEMU (x86_64, KVM, kvm=off with the hv-* enlightenments,
vmbus-bridge and hv-balloon) with KASAN, hv_vmbus and hv_balloon as
modules loaded from init after failslab is armed, on mainline
d135ab453a74. failslab is limited to allocations under
vmbus_device_register(), and fails the N-th one when the hv-balloon
offer is processed. Without the series, every run hits the BUG_ON:
24 of 24 with the failure in device_register(), and 30 of 30 with it in
the channels kset, the channel's kobject_add() or its sysfs group.
With both patches, all 54 runs leave the offer unbound with "unable to
add child device object" and report nothing (no BUG, KASAN, WARNING or
refcount_t message). The sub-channel path of patch 1 is not covered:
QEMU's VMBus offers no sub-channels.

The failures need an allocation failure while the guest processes a
channel offer from the host.

Yuqiang Sun (2):
  Drivers: hv: vmbus: Keep the channel when vmbus_add_channel_kobj()
    fails
  Drivers: hv: vmbus: Do not remove the channel when device registration
    fails

 drivers/hv/channel_mgmt.c |  5 +++--
 drivers/hv/hyperv_vmbus.h |  1 +
 drivers/hv/vmbus_drv.c    | 43 +++++++++++++++++++++++++++++----------
 3 files changed, 36 insertions(+), 13 deletions(-)


base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.53.0

[PATCH 1/2] Drivers: hv: vmbus: Keep the channel when vmbus_add_channel_kobj() fails

[Yuqiang Sun]

The channel is freed by the release of its kobject, and free_channel()
drops that reference. Since commit 8bc69f86328e ("Drivers: hv: vmbus:
Fix memory leak in vmbus_add_channel_kobj"), vmbus_add_channel_kobj()
also drops it when it fails, as kobject_init_and_add() asks for. That
frees the channel, and both callers then go on with it:
vmbus_add_channel_work() unlinks a sub-channel, unmaps and releases its
relid and calls free_channel(), a use after free and a second
kobject_put(); for a primary channel, vmbus_device_register() unwinds
through vmbus_device_release(), which removes the channel too.

The kobject is also only initialized in vmbus_add_channel_kobj(), so the
error paths before it, for instance a vmbus_device_create() failure in
vmbus_add_channel_work(), call free_channel() on a kobject that was
never initialized: kobject_put() warns and does nothing, so the channel
is leaked, and removing the attribute group from a kobject that is not
in sysfs warns as well.

Initialize the kobject when the channel is allocated, so that
free_channel() is the one place that drops its reference on every path.
vmbus_add_channel_kobj() then only adds it, and leaves the reference to
its callers when it fails. Remove the attribute group only if the
kobject was added.

Fixes: c2e5df616e1a ("vmbus: add per-channel sysfs info")
Fixes: 8bc69f86328e ("Drivers: hv: vmbus: Fix memory leak in vmbus_add_channel_kobj")
Cc: stable@vger.kernel.org
Signed-off-by: Yuqiang Sun <izaiahsun@gmail.com>
---

Notes:
    The bug was found with kbound, a research static analysis prototype.
    The patch, the changelog and the test setup were written with the help
    of an AI coding assistant (Claude); I have reviewed them and tested them
    as described in the cover letter.

 drivers/hv/channel_mgmt.c |  5 +++--
 drivers/hv/hyperv_vmbus.h |  1 +
 drivers/hv/vmbus_drv.c    | 25 ++++++++++++++++++-------
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c42def04ac41..cb1d8d76928f 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -367,6 +367,7 @@ static struct vmbus_channel *alloc_channel(void)
 		     vmbus_on_event, (unsigned long)channel);
 
 	hv_ringbuffer_pre_init(channel);
+	vmbus_init_channel_kobj(channel);
 
 	return channel;
 }
@@ -663,8 +664,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 			mutex_unlock(&vmbus_connection.channel_mutex);
 			cpus_read_unlock();
 			/*
-			 * Don't call free_channel(), because newchannel->kobj
-			 * is not initialized yet.
+			 * Don't call free_channel(): newchannel was never
+			 * added, and its kobject holds nothing yet.
 			 */
 			kfree(newchannel);
 			WARN_ON_ONCE(1);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 33923621a5a3..7865a519dd7f 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -408,6 +408,7 @@ struct hv_device *vmbus_device_create(const guid_t *type,
 
 int vmbus_device_register(struct hv_device *child_device_obj);
 void vmbus_device_unregister(struct hv_device *device_obj);
+void vmbus_init_channel_kobj(struct vmbus_channel *channel);
 int vmbus_add_channel_kobj(struct hv_device *device_obj,
 			   struct vmbus_channel *channel);
 
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4692687cdbf3..fc0f16f51266 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2084,6 +2084,15 @@ int hv_remove_ring_sysfs(struct vmbus_channel *channel)
 }
 EXPORT_SYMBOL_GPL(hv_remove_ring_sysfs);
 
+/*
+ * vmbus_init_channel_kobj - initialize the kobject that holds the channel's
+ * lifetime; free_channel() drops its reference
+ */
+void vmbus_init_channel_kobj(struct vmbus_channel *channel)
+{
+	kobject_init(&channel->kobj, &vmbus_chan_ktype);
+}
+
 /*
  * vmbus_add_channel_kobj - setup a sub-directory under device/channels
  */
@@ -2094,13 +2103,14 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 	u32 relid = channel->offermsg.child_relid;
 	int ret;
 
+	/*
+	 * On failure the kobject keeps its reference: the callers' error
+	 * paths drop it in free_channel(), which also frees the channel.
+	 */
 	kobj->kset = dev->channels_kset;
-	ret = kobject_init_and_add(kobj, &vmbus_chan_ktype, NULL,
-				   "%u", relid);
-	if (ret) {
-		kobject_put(kobj);
+	ret = kobject_add(kobj, NULL, "%u", relid);
+	if (ret)
 		return ret;
-	}
 
 	ret = sysfs_create_group(kobj, &vmbus_chan_group);
 
@@ -2109,7 +2119,6 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
 		 * The calling functions' error handling paths will cleanup the
 		 * empty channel directory.
 		 */
-		kobject_put(kobj);
 		dev_err(device, "Unable to set up channel sysfs files\n");
 		return ret;
 	}
@@ -2124,7 +2133,9 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
  */
 void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
 {
-	sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
+	/* A channel that failed before vmbus_add_channel_kobj() has none */
+	if (channel->kobj.state_in_sysfs)
+		sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
 }
 
 /*
-- 
2.53.0

[PATCH 2/2] Drivers: hv: vmbus: Do not remove the channel when device registration fails

[Yuqiang Sun]

When vmbus_device_register() fails, it drops the device it was setting
up, and the device's release, vmbus_device_release(), calls
hv_process_channel_removal() on the channel. That function is for
channels the host has rescinded and starts with
BUG_ON(!channel->rescind), so the kernel stops there. Without the
BUG_ON, it would unmap and release the relid, unlink the channel and
free it, and then the error path of vmbus_add_channel_work() would do
all of that again.

Leave the channel to vmbus_add_channel_work(): clear the device's
channel pointer before dropping the device on the error paths of
vmbus_device_register(), and only remove the channel in
vmbus_device_release() if there is one. If a driver was bound, its
remove callback runs in device_del() and still sees the channel.

Fixes: c2e5df616e1a ("vmbus: add per-channel sysfs info")
Cc: stable@vger.kernel.org
Signed-off-by: Yuqiang Sun <izaiahsun@gmail.com>
---

Notes:
    The bug was found with kbound, a research static analysis prototype.
    The patch, the changelog and the test setup were written with the help
    of an AI coding assistant (Claude); I have reviewed them and tested them
    as described in the cover letter.

 drivers/hv/vmbus_drv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index fc0f16f51266..e8fb0c4c4f5e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -979,9 +979,15 @@ static void vmbus_device_release(struct device *device)
 
 	hv_debug_rm_dev_dir(hv_dev);
 
-	mutex_lock(&vmbus_connection.channel_mutex);
-	hv_process_channel_removal(channel);
-	mutex_unlock(&vmbus_connection.channel_mutex);
+	/*
+	 * No channel if vmbus_device_register() failed: the channel was not
+	 * rescinded, and vmbus_add_channel_work() cleans it up.
+	 */
+	if (channel) {
+		mutex_lock(&vmbus_connection.channel_mutex);
+		hv_process_channel_removal(channel);
+		mutex_unlock(&vmbus_connection.channel_mutex);
+	}
 	kfree(hv_dev);
 }
 
@@ -2189,6 +2195,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 	ret = device_register(&child_device_obj->device);
 	if (ret) {
 		pr_err("Unable to register child device\n");
+		child_device_obj->channel = NULL;
 		put_device(&child_device_obj->device);
 		return ret;
 	}
@@ -2228,7 +2235,10 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 	kset_unregister(child_device_obj->channels_kset);
 
 err_dev_unregister:
-	device_unregister(&child_device_obj->device);
+	/* The driver's remove, if one was bound, still needs the channel */
+	device_del(&child_device_obj->device);
+	child_device_obj->channel = NULL;
+	put_device(&child_device_obj->device);
 	return ret;
 }
 
-- 
2.53.0

Re: [PATCH 2/2] Drivers: hv: vmbus: Do not remove the channel when device registration fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Drivers: hv: vmbus: channel UAF after device_del() in vmbus_device_register()
- [High] Drivers: hv: vmbus: NULL channel dereference in vmbus_match() on reg failure
--

commit 1b8392611b72249068d7b640dddf153a43c0780f
Author: Yuqiang Sun <izaiahsun@gmail.com>

Drivers: hv: vmbus: Do not remove the channel when device registration fails

This commit clears child_device_obj->channel before dropping the device on
the error paths of vmbus_device_register(), and updates
vmbus_device_release() to only call hv_process_channel_removal() if the
channel pointer is non-NULL, leaving channel cleanup on registration failure
to vmbus_add_channel_work().

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index fc0f16f512669..e8fb0c4c4f5ea 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c

[ ... ]

> @@ -2189,6 +2195,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	ret = device_register(&child_device_obj->device);
>  	if (ret) {
>  		pr_err("Unable to register child device\n");
> +		child_device_obj->channel = NULL;
>  		put_device(&child_device_obj->device);
>  		return ret;
>  	}

[Severity: High]
Can clearing child_device_obj->channel here (and in err_dev_unregister below)
cause a NULL pointer dereference in vmbus_match() if a concurrent
driver_attach() is iterating over hv_bus?

Once device_register() -> device_add() -> bus_add_device() adds
child_device_obj->device.p->knode_bus to hv_bus's klist_devices, a concurrent
driver_attach() (for example from __vmbus_driver_register() or userspace
writing to new_id_store() -> vmbus_add_dynid()) can iterate over
klist_devices via bus_for_each_dev() while holding an active klist node
reference on child_device_obj->device.

If device_add() subsequently fails at its DPMError label (or if
vmbus_device_register() fails later and calls device_del() at
err_dev_unregister), bus_remove_device() calls klist_del(&dev->p->knode_bus).
Unlike klist_remove(), klist_del() does not wait for active klist_iter
readers in bus_for_each_dev() to finish before vmbus_device_register() sets
child_device_obj->channel = NULL.

On the concurrent bus_for_each_dev() thread, __driver_attach() calls
driver_match_device() -> vmbus_match() before acquiring device_lock(dev) or
checking dev->p->dead:

drivers/hv/vmbus_drv.c:vmbus_match() {
	...
	/* The hv_sock driver handles all hv_sock offers. */
	if (is_hvsock_channel(hv_dev->channel))
		return drv->hvsock;
	...
}

Unlike the sysfs attribute callbacks in drivers/hv/vmbus_drv.c which check
for !hv_dev->channel, vmbus_match() unconditionally passes hv_dev->channel to
is_hvsock_channel(), dereferencing hv_dev->channel->offermsg when
hv_dev->channel is NULL.

> @@ -2228,7 +2235,10 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	kset_unregister(child_device_obj->channels_kset);
>  
>  err_dev_unregister:
> -	device_unregister(&child_device_obj->device);
> +	/* The driver's remove, if one was bound, still needs the channel */
> +	device_del(&child_device_obj->device);
> +	child_device_obj->channel = NULL;
> +	put_device(&child_device_obj->device);
>  	return ret;
>  }

[Severity: Critical]
When vmbus_device_register() fails after device_register() has already
probed and bound a driver, does device_del() followed by freeing newchannel
in vmbus_add_channel_work()'s err_deq_chan path leave dangling channel
pointers when the channel has not been rescinded?

There appear to be two cases where drv->remove() does not sever all
references created during drv->probe() unless the channel was rescinded:

1. For hv_sock channels, hvs_probe() -> hvs_open_connection() in
net/vmw_vsock/hyperv_transport.c sets hvs->chan = chan (or
hvs_new->chan = chan) on the vsock_sock, takes a socket reference via
sock_hold(), and inserts the socket into the vsock connected table and accept
queue. When device_del() invokes vmbus_remove() -> hvs_remove():

net/vmw_vsock/hyperv_transport.c:hvs_remove() {
	struct vmbus_channel *chan = hdev->channel;

	vmbus_close(chan);
}

hvs_remove() only closes the ring buffer because in normal operation it is
only called from hvs_destruct() -> vmbus_hvsock_device_unregister() after
hvs_close_connection() has already run on channel rescind. When
err_dev_unregister runs without a rescind, hvs_close_connection() is never
called, so the live vsock_sock retains hvs->chan pointing to newchannel
after vmbus_add_channel_work() frees it via free_channel(newchannel), leading
to a use-after-free in socket operations or in hvs_destruct() ->
vmbus_hvsock_device_unregister(chan).

2. For multichannel drivers that open sub-channels during probe,
vmbus_process_offer() links offered sub-channels onto newchannel->sc_list
with sc->primary_channel = newchannel and queues vmbus_add_channel_work()
onto handle_sub_chan_wq. When device_del() runs drv->remove() ->
vmbus_close() -> vmbus_disconnect_ring():

drivers/hv/channel.c:vmbus_disconnect_ring() {
	...
	list_for_each_entry_safe(cur_channel, tmp, &channel->sc_list, sc_list) {
		if (cur_channel->rescind)
			wait_for_completion(&cur_channel->rescind_event);

		mutex_lock(&vmbus_connection.channel_mutex);
		if (vmbus_close_internal(cur_channel) == 0) {
			vmbus_free_ring(cur_channel);

			if (cur_channel->rescind)
				hv_process_channel_removal(cur_channel);
		}
		mutex_unlock(&vmbus_connection.channel_mutex);
	}
	...
}

vmbus_disconnect_ring() only removes cur_channel if cur_channel->rescind is
true, leaving non-rescinded sub-channels on sc_list and handle_sub_chan_wq
referencing the freed primary channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102094719.35760-1-izaiahsun@gmail.com?part=2