Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet

commit 07a756a49f4b4290b49ea46e089cbe6f79ff8d26 upstream.

If the KVP (or VSS) daemon starts before the VMBus channel's ringbuffer is
fully initialized, we can hit the panic below:

hv_utils: Registering HyperV Utility Driver
hv_vmbus: registering driver hv_utils
...
BUG: kernel NULL pointer dereference, address: 0000000000000000
CPU: 44 UID: 0 PID: 2552 Comm: hv_kvp_daemon Tainted: G E 6.11.0-rc3+ #1
RIP: 0010:hv_pkt_iter_first+0x12/0xd0
Call Trace:
...
 vmbus_recvpacket
 hv_kvp_onchannelcallback
 vmbus_on_event
 tasklet_action_common
 tasklet_action
 handle_softirqs
 irq_exit_rcu
 sysvec_hyperv_stimer0
 </IRQ>
 <TASK>
 asm_sysvec_hyperv_stimer0
...
 kvp_register_done
 hvt_op_read
 vfs_read
 ksys_read
 __x64_sys_read

This can happen because the KVP/VSS channel callback can be invoked
even before the channel is fully opened:
1) as soon as hv_kvp_init() -> hvutil_transport_init() creates
/dev/vmbus/hv_kvp, the kvp daemon can open the device file immediately and
register itself to the driver by writing a message KVP_OP_REGISTER1 to the
file (which is handled by kvp_on_msg() ->kvp_handle_handshake()) and
reading the file for the driver's response, which is handled by
hvt_op_read(), which calls hvt->on_read(), i.e. kvp_register_done().

2) the problem with kvp_register_done() is that it can cause the
channel callback to be called even before the channel is fully opened,
and when the channel callback is starting to run, util_probe()->
vmbus_open() may have not initialized the ringbuffer yet, so the
callback can hit the panic of NULL pointer dereference.

To reproduce the panic consistently, we can add a "ssleep(10)" for KVP in
__vmbus_open(), just before the first hv_ringbuffer_init(), and then we
unload and reload the driver hv_utils, and run the daemon manually within
the 10 seconds.

Fix the panic by reordering the steps in util_probe() so the char dev
entry used by the KVP or VSS daemon is not created until after
vmbus_open() has completed. This reordering prevents the race condition
from happening.

Reported-by: Dexuan Cui <decui@microsoft.com>
Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons registration")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Acked-by: Wei Liu <wei.liu@kernel.org>
Link: https://lore.kernel.org/r/20241106154247.2271-3-mhklinux@outlook.com
Signed-off-by: Wei Liu <wei.liu@kernel.org>
Message-ID: <20241106154247.2271-3-mhklinux@outlook.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index d35b60c0611486c8c909d2f8f7c730ff913df30d..77017d9518267c3f45cbe87952314a0dd1351b1b 100644 (file)
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -767,6 +767,12 @@ hv_kvp_init(struct hv_util_service *srv)
         */
        kvp_transaction.state = HVUTIL_DEVICE_INIT;
 
+       return 0;
+}
+
+int
+hv_kvp_init_transport(void)
+{
        hvt = hvutil_transport_init(kvp_devname, CN_KVP_IDX, CN_KVP_VAL,
                                    kvp_on_msg, kvp_on_reset);
        if (!hvt)

diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 0d2184be16912559a8cfa784c762e6418ebd3279..397f4c8fa46c315a6875abbd26307f25d6665cfb 100644 (file)
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -388,6 +388,12 @@ hv_vss_init(struct hv_util_service *srv)
         */
        vss_transaction.state = HVUTIL_DEVICE_INIT;
 
+       return 0;
+}
+
+int
+hv_vss_init_transport(void)
+{
        hvt = hvutil_transport_init(vss_devname, CN_VSS_IDX, CN_VSS_VAL,
                                    vss_on_msg, vss_on_reset);
        if (!hvt) {

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 835e6039c186789433f54562378f43df8851fabb..ad6c066fd2b758bec6cd7cf6f02bac56cbee047c 100644 (file)
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -141,6 +141,7 @@ static struct hv_util_service util_heartbeat = {
 static struct hv_util_service util_kvp = {
        .util_cb = hv_kvp_onchannelcallback,
        .util_init = hv_kvp_init,
+       .util_init_transport = hv_kvp_init_transport,
        .util_pre_suspend = hv_kvp_pre_suspend,
        .util_pre_resume = hv_kvp_pre_resume,
        .util_deinit = hv_kvp_deinit,
@@ -149,6 +150,7 @@ static struct hv_util_service util_kvp = {
 static struct hv_util_service util_vss = {
        .util_cb = hv_vss_onchannelcallback,
        .util_init = hv_vss_init,
+       .util_init_transport = hv_vss_init_transport,
        .util_pre_suspend = hv_vss_pre_suspend,
        .util_pre_resume = hv_vss_pre_resume,
        .util_deinit = hv_vss_deinit,
@@ -592,6 +594,13 @@ static int util_probe(struct hv_device *dev,
        if (ret)
                goto error;
 
+       if (srv->util_init_transport) {
+               ret = srv->util_init_transport();
+               if (ret) {
+                       vmbus_close(dev->channel);
+                       goto error;
+               }
+       }
        return 0;
 
 error:

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index dc673edf053c3b7dc6f086f946a3290242c90a36..ab12e21bf5fc7bb81329cefb0ae1794a7402db1a 100644 (file)
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -365,12 +365,14 @@ void vmbus_on_event(unsigned long data);
 void vmbus_on_msg_dpc(unsigned long data);
 
 int hv_kvp_init(struct hv_util_service *srv);
+int hv_kvp_init_transport(void);
 void hv_kvp_deinit(void);
 int hv_kvp_pre_suspend(void);
 int hv_kvp_pre_resume(void);
 void hv_kvp_onchannelcallback(void *context);
 
 int hv_vss_init(struct hv_util_service *srv);
+int hv_vss_init_transport(void);
 void hv_vss_deinit(void);
 int hv_vss_pre_suspend(void);
 int hv_vss_pre_resume(void);

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 811d59cf891ba9a9ea3f24a415dabde41b58077b..afc20437090157490321c0e428241c087675796c 100644 (file)
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1567,6 +1567,7 @@ struct hv_util_service {
        void *channel;
        void (*util_cb)(void *);
        int (*util_init)(struct hv_util_service *);
+       int (*util_init_transport)(void);
        void (*util_deinit)(void);
        int (*util_pre_suspend)(void);
        int (*util_pre_resume)(void);