From bc0dcbc5c2c539f37004f2cce0e6e245b2e50b6c Mon Sep 17 00:00:00 2001 From: Srujana Challa Date: Mon, 16 Sep 2024 21:52:55 +0530 Subject: [PATCH 1/5] vdpa/octeon_ep: Fix format specifier for pointers in debug messages Updates the debug messages in octep_vdpa_hw.c to use the %p format specifier for pointers instead of casting them to u64. Fixes smatch warning: octep_hw_caps_read() warn: argument 3 to %016llx specifier is cast from pointer Fixes: 8b6c724cdab8 ("virtio: vdpa: vDPA driver for Marvell OCTEON DPU devices") Reported-by: kernel test robot Reported-by: Dan Carpenter Closes: https://lore.kernel.org/r/202409160431.bRhZWhiU-lkp@intel.com/ Signed-off-by: Srujana Challa Message-Id: <20240916162255.677774-1-schalla@marvell.com> Signed-off-by: Michael S. Tsirkin --- drivers/vdpa/octeon_ep/octep_vdpa_hw.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c index 11bd76ae18cf..1d4767b33315 100644 --- a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c +++ b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c @@ -475,11 +475,11 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev) dev_err(dev, "Incomplete PCI capabilities"); return -EIO; } - dev_info(dev, "common cfg mapped at: 0x%016llx\n", (u64)(uintptr_t)oct_hw->common_cfg); - dev_info(dev, "device cfg mapped at: 0x%016llx\n", (u64)(uintptr_t)oct_hw->dev_cfg); - dev_info(dev, "isr cfg mapped at: 0x%016llx\n", (u64)(uintptr_t)oct_hw->isr); - dev_info(dev, "notify base: 0x%016llx, notify off multiplier: %u\n", - (u64)(uintptr_t)oct_hw->notify_base, oct_hw->notify_off_multiplier); + dev_info(dev, "common cfg mapped at: %p\n", oct_hw->common_cfg); + dev_info(dev, "device cfg mapped at: %p\n", oct_hw->dev_cfg); + dev_info(dev, "isr cfg mapped at: %p\n", oct_hw->isr); + dev_info(dev, "notify base: %p, notify off multiplier: %u\n", + oct_hw->notify_base, oct_hw->notify_off_multiplier); oct_hw->config_size = octep_get_config_size(oct_hw); oct_hw->features = octep_hw_get_dev_features(oct_hw); @@ -511,7 +511,7 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev) } mbox = octep_get_mbox(oct_hw); octep_mbox_init(mbox); - dev_info(dev, "mbox mapped at: 0x%016llx\n", (u64)(uintptr_t)mbox); + dev_info(dev, "mbox mapped at: %p\n", mbox); return 0; } From 83c334ed521638c8dffe545ddf49d61430680308 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 12 Sep 2024 10:37:53 -0400 Subject: [PATCH 2/5] virtio_ring: tag event_triggered as racy for KCSAN Setting event_triggered from the interrupt handler is fundamentally racy. There are races of 2 types: 1. vq processing can read false value while interrupt triggered and set it to true. result will be a bit of extra work when disabling cbs, no big deal. 1. vq processing can set false value then interrupt immediately sets true value since interrupt then triggers a callback which will process buffers, this is also not an issue. However, looks like KCSAN can not figure all this out, and warns about the race between the write and the read. Tag the access data_racy for now. We should probably look at ways to make this more straight-forwardly correct. Message-ID: <6bdd771a4fb7625a9227971b3cf4745c34c31a32.1726153334.git.mst@redhat.com> Reported-by: syzbot+8a02104389c2e0ef5049@syzkaller.appspotmail.com Signed-off-by: Michael S. Tsirkin Acked-by: Jason Wang Acked-by: Marco Elver --- drivers/virtio/virtio_ring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index be7309b1e860..98374ed7c577 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -2588,7 +2588,7 @@ irqreturn_t vring_interrupt(int irq, void *_vq) /* Just a hint for performance: so it's ok that this can be racy! */ if (vq->event) - vq->event_triggered = true; + data_race(vq->event_triggered = true); pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback); if (vq->vq.callback) From b9efbe2b8f0177fa97bfab290d60858900aa196b Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 16 Sep 2024 14:16:44 -0400 Subject: [PATCH 3/5] virtio_console: fix misc probe bugs This fixes the following issue discovered by code review: after vqs have been created, a buggy device can send an interrupt. A control vq callback will then try to schedule control_work which has not been initialized yet. Similarly for config interrupt. Further, in and out vq callbacks invoke find_port_by_vq which attempts to take ports_lock which also has not been initialized. To fix, init all locks and work before creating vqs. Message-ID: Fixes: 17634ba25544 ("virtio: console: Add a new MULTIPORT feature, support for generic ports") Signed-off-by: Michael S. Tsirkin --- drivers/char/virtio_console.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index de7d720d99fa..bcb05fc44c99 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -2007,25 +2007,27 @@ static int virtcons_probe(struct virtio_device *vdev) multiport = true; } - err = init_vqs(portdev); - if (err < 0) { - dev_err(&vdev->dev, "Error %d initializing vqs\n", err); - goto free_chrdev; - } - spin_lock_init(&portdev->ports_lock); INIT_LIST_HEAD(&portdev->ports); INIT_LIST_HEAD(&portdev->list); - virtio_device_ready(portdev->vdev); - INIT_WORK(&portdev->config_work, &config_work_handler); INIT_WORK(&portdev->control_work, &control_work_handler); if (multiport) { spin_lock_init(&portdev->c_ivq_lock); spin_lock_init(&portdev->c_ovq_lock); + } + err = init_vqs(portdev); + if (err < 0) { + dev_err(&vdev->dev, "Error %d initializing vqs\n", err); + goto free_chrdev; + } + + virtio_device_ready(portdev->vdev); + + if (multiport) { err = fill_queue(portdev->c_ivq, &portdev->c_ivq_lock); if (err < 0) { dev_err(&vdev->dev, From a194c985973276b2f280428c848f20369bb83734 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Wed, 2 Oct 2024 09:35:20 -0400 Subject: [PATCH 4/5] vsock/virtio: use GFP_ATOMIC under RCU read lock virtio_transport_send_pkt in now called on transport fast path, under RCU read lock. In that case, we have a bug: virtio_add_sgs is called with GFP_KERNEL, and might sleep. Pass the gfp flags as an argument, and use GFP_ATOMIC on the fast path. Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty") Reported-by: Christian Brauner Cc: Stefano Garzarella Cc: Luigi Leonardi Message-ID: <3fbfb6e871f625f89eb578c7228e127437b1975a.1727876449.git.mst@redhat.com> Signed-off-by: Michael S. Tsirkin Reviewed-by: Pankaj Gupta Reviewed-by: Christian Brauner Reviewed-by: Luigi Leonardi Reviewed-by: Stefano Garzarella --- net/vmw_vsock/virtio_transport.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index f992f9a216f0..0cd965f24609 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -96,7 +96,7 @@ out_rcu: /* Caller need to hold vsock->tx_lock on vq */ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq, - struct virtio_vsock *vsock) + struct virtio_vsock *vsock, gfp_t gfp) { int ret, in_sg = 0, out_sg = 0; struct scatterlist **sgs; @@ -140,7 +140,7 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq, } } - ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL); + ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp); /* Usually this means that there is no more space available in * the vq */ @@ -178,7 +178,7 @@ virtio_transport_send_pkt_work(struct work_struct *work) reply = virtio_vsock_skb_reply(skb); - ret = virtio_transport_send_skb(skb, vq, vsock); + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL); if (ret < 0) { virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb); break; @@ -221,7 +221,7 @@ static int virtio_transport_send_skb_fast_path(struct virtio_vsock *vsock, struc if (unlikely(ret == 0)) return -EBUSY; - ret = virtio_transport_send_skb(skb, vq, vsock); + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_ATOMIC); if (ret == 0) virtqueue_kick(vq); From 221af82f606d928ccef19a16d35633c63026f1be Mon Sep 17 00:00:00 2001 From: Haoran Zhang Date: Tue, 1 Oct 2024 15:14:15 -0500 Subject: [PATCH 5/5] vhost/scsi: null-ptr-dereference in vhost_scsi_get_req() Since commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from control queue handler") a null pointer dereference bug can be triggered when guest sends an SCSI AN request. In vhost_scsi_ctl_handle_vq(), `vc.target` is assigned with `&v_req.tmf.lun[1]` within a switch-case block and is then passed to vhost_scsi_get_req() which extracts `vc->req` and `tpg`. However, for a `VIRTIO_SCSI_T_AN_*` request, tpg is not required, so `vc.target` is set to NULL in this branch. Later, in vhost_scsi_get_req(), `vc->target` is dereferenced without being checked, leading to a null pointer dereference bug. This bug can be triggered from guest. When this bug occurs, the vhost_worker process is killed while holding `vq->mutex` and the corresponding tpg will remain occupied indefinitely. Below is the KASAN report: Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN NOPTI KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] CPU: 1 PID: 840 Comm: poc Not tainted 6.10.0+ #1 Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 RIP: 0010:vhost_scsi_get_req+0x165/0x3a0 Code: 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 2b 02 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 65 30 4c 89 e2 48 c1 ea 03 <0f> b6 04 02 4c 89 e2 83 e2 07 38 d0 7f 08 84 c0 0f 85 be 01 00 00 RSP: 0018:ffff888017affb50 EFLAGS: 00010246 RAX: dffffc0000000000 RBX: ffff88801b000000 RCX: 0000000000000000 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff888017affcb8 RBP: ffff888017affb80 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: ffff888017affc88 R14: ffff888017affd1c R15: ffff888017993000 FS: 000055556e076500(0000) GS:ffff88806b100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000200027c0 CR3: 0000000010ed0004 CR4: 0000000000370ef0 Call Trace: ? show_regs+0x86/0xa0 ? die_addr+0x4b/0xd0 ? exc_general_protection+0x163/0x260 ? asm_exc_general_protection+0x27/0x30 ? vhost_scsi_get_req+0x165/0x3a0 vhost_scsi_ctl_handle_vq+0x2a4/0xca0 ? __pfx_vhost_scsi_ctl_handle_vq+0x10/0x10 ? __switch_to+0x721/0xeb0 ? __schedule+0xda5/0x5710 ? __kasan_check_write+0x14/0x30 ? _raw_spin_lock+0x82/0xf0 vhost_scsi_ctl_handle_kick+0x52/0x90 vhost_run_work_list+0x134/0x1b0 vhost_task_fn+0x121/0x350 ... ---[ end trace 0000000000000000 ]--- Let's add a check in vhost_scsi_get_req. Fixes: 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from control queue handler") Signed-off-by: Haoran Zhang [whitespace fixes] Signed-off-by: Mike Christie Message-Id: Signed-off-by: Michael S. Tsirkin --- drivers/vhost/scsi.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 006ffacf1c56..c50f3fb49a66 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1029,20 +1029,23 @@ vhost_scsi_get_req(struct vhost_virtqueue *vq, struct vhost_scsi_ctx *vc, /* virtio-scsi spec requires byte 0 of the lun to be 1 */ vq_err(vq, "Illegal virtio-scsi lun: %u\n", *vc->lunp); } else { - struct vhost_scsi_tpg **vs_tpg, *tpg; + struct vhost_scsi_tpg **vs_tpg, *tpg = NULL; - vs_tpg = vhost_vq_get_backend(vq); /* validated at handler entry */ - - tpg = READ_ONCE(vs_tpg[*vc->target]); - if (unlikely(!tpg)) { - vq_err(vq, "Target 0x%x does not exist\n", *vc->target); - } else { - if (tpgp) - *tpgp = tpg; - ret = 0; + if (vc->target) { + /* validated at handler entry */ + vs_tpg = vhost_vq_get_backend(vq); + tpg = READ_ONCE(vs_tpg[*vc->target]); + if (unlikely(!tpg)) { + vq_err(vq, "Target 0x%x does not exist\n", *vc->target); + goto out; + } } - } + if (tpgp) + *tpgp = tpg; + ret = 0; + } +out: return ret; }