netdev - Re: [RFC PATCH vhost] vhost-vdpa: Fix invalid irq bypass unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51e9ed8f37a1b5fbee9603905b925aedec712131.camel@nvidia.com>
Date: Fri, 2 Aug 2024 06:51:10 +0000
From: Dragos Tatulea <dtatulea@...dia.com>
To: "jasowang@...hat.com" <jasowang@...hat.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "mst@...hat.com"
	<mst@...hat.com>, "eperezma@...hat.com" <eperezma@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH vhost] vhost-vdpa: Fix invalid irq bypass unregister

On Fri, 2024-08-02 at 11:29 +0800, Jason Wang wrote:
> On Thu, Aug 1, 2024 at 11:38 PM Dragos Tatulea <dtatulea@...dia.com> wrote:
> > 
> > The following workflow triggers the crash referenced below:
> > 
> > 1) vhost_vdpa_unsetup_vq_irq() unregisters the irq bypass producer
> >    but the producer->token is still valid.
> > 2) vq context gets released and reassigned to another vq.
> 
> Just to make sure I understand here, which structure is referred to as
> "vq context" here? I guess it's not call_ctx as it is a part of the vq
> itself.
> 
> > 3) That other vq registers it's producer with the same vq context
> >    pointer as token in vhost_vdpa_setup_vq_irq().
> 
> Or did you mean when a single eventfd is shared among different vqs?
> 
Yes, that's what I mean: vq->call_ctx.ctx which is a eventfd_ctx.

But I don't think it's shared in this case, only that the old eventfd_ctx value
is lingering in producer->token. And this old eventfd_ctx is assigned now to
another vq.

> > 4) The original vq tries to unregister it's producer which it has
> >    already unlinked in step 1. irq_bypass_unregister_producer() will go
> >    ahead and unlink the producer once again. That happens because:
> >       a) The producer has a token.
> >       b) An element with that token is found. But that element comes
> >          from step 3.
> > 
> > I see 3 ways to fix this:
> > 1) Fix the vhost-vdpa part. What this patch does. vfio has a different
> >    workflow.
> > 2) Set the token to NULL directly in irq_bypass_unregister_producer()
> >    after unlinking the producer. But that makes the API asymmetrical.
> > 3) Make irq_bypass_unregister_producer() also compare the pointer
> >    elements not just the tokens and do the unlink only on match.
> > 
> > Any thoughts?
> > 
> > Oops: general protection fault, probably for non-canonical address 0xdead000000000108: 0000 [#1] SMP
> > CPU: 8 PID: 5190 Comm: qemu-system-x86 Not tainted 6.10.0-rc7+ #6
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> > RIP: 0010:irq_bypass_unregister_producer+0xa5/0xd0
> > RSP: 0018:ffffc900034d7e50 EFLAGS: 00010246
> > RAX: dead000000000122 RBX: ffff888353d12718 RCX: ffff88810336a000
> > RDX: dead000000000100 RSI: ffffffff829243a0 RDI: 0000000000000000
> > RBP: ffff888353c42000 R08: ffff888104882738 R09: ffff88810336a000
> > R10: ffff888448ab2050 R11: 0000000000000000 R12: ffff888353d126a0
> > R13: 0000000000000004 R14: 0000000000000055 R15: 0000000000000004
> > FS:  00007f9df9403c80(0000) GS:ffff88852cc00000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000562dffc6b568 CR3: 000000012efbb006 CR4: 0000000000772ef0
> > PKRU: 55555554
> > Call Trace:
> >  <TASK>
> >  ? die_addr+0x36/0x90
> >  ? exc_general_protection+0x1a8/0x390
> >  ? asm_exc_general_protection+0x26/0x30
> >  ? irq_bypass_unregister_producer+0xa5/0xd0
> >  vhost_vdpa_setup_vq_irq+0x5a/0xc0 [vhost_vdpa]
> >  vhost_vdpa_unlocked_ioctl+0xdcd/0xe00 [vhost_vdpa]
> >  ? vhost_vdpa_config_cb+0x30/0x30 [vhost_vdpa]
> >  __x64_sys_ioctl+0x90/0xc0
> >  do_syscall_64+0x4f/0x110
> >  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > RIP: 0033:0x7f9df930774f
> > RSP: 002b:00007ffc55013080 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > RAX: ffffffffffffffda RBX: 0000562dfe134d20 RCX: 00007f9df930774f
> > RDX: 00007ffc55013200 RSI: 000000004008af21 RDI: 0000000000000011
> > RBP: 00007ffc55013200 R08: 0000000000000002 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 0000562dfe134360
> > R13: 0000562dfe134d20 R14: 0000000000000000 R15: 00007f9df801e190
> > 
> > Signed-off-by: Dragos Tatulea <dtatulea@...dia.com>
> > ---
> >  drivers/vhost/vdpa.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 478cd46a49ed..d4a7a3918d86 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -226,6 +226,7 @@ static void vhost_vdpa_unsetup_vq_irq(struct vhost_vdpa *v, u16 qid)
> >         struct vhost_virtqueue *vq = &v->vqs[qid];
> > 
> >         irq_bypass_unregister_producer(&vq->call_ctx.producer);
> > +       vq->call_ctx.producer.token = NULL;
> >  }
> > 
> >  static int _compat_vdpa_reset(struct vhost_vdpa *v)
> > --
> > 2.45.2
> > 
> 
Thanks