From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965495AbdCWQGt (ORCPT ); Thu, 23 Mar 2017 12:06:49 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:45273 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965368AbdCWQGs (ORCPT ); Thu, 23 Mar 2017 12:06:48 -0400 Date: Thu, 23 Mar 2017 17:06:35 +0100 From: Cornelia Huck To: David Hildenbrand Cc: kvm@vger.kernel.org, Paolo Bonzini , rkrcmar@redhat.com, Dmitry Vyukov , Marcelo Tosatti , stable@vger.kernel.org, LKML Subject: Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail In-Reply-To: <20170323143441.5749-1-david@redhat.com> References: <20170323143441.5749-1-david@redhat.com> Organization: IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz =?UTF-8?B?R2VzY2jDpGZ0c2bDvGhydW5nOg==?= Dirk Wittkopp Sitz der Gesellschaft: =?UTF-8?B?QsO2Ymxpbmdlbg==?= Registergericht: Amtsgericht Stuttgart, HRB 243294 X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17032316-0020-0000-0000-0000032CBA05 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17032316-0021-0000-0000-000040EE351A Message-Id: <20170323170635.1fb91a2c.cornelia.huck@de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-23_13:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703230141 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 23 Mar 2017 15:34:41 +0100 David Hildenbrand wrote: > No caller currently checks the return value of > kvm_io_bus_unregister_dev(). This is evil, as all callers silently go on > freeing their device. A stale reference will remain in the io_bus, > getting at least used again, when the iobus gets teared down on > kvm_destroy_vm() - leading to use after free errors. > > There is nothing the callers could do, except retrying over and over > again. > > So let's simply remove the bus altogether, print an error and make > sure no one can access this broken bus again (returning -ENOMEM on any > attempt to access it). > > Fixes: e93f8a0f821e ("KVM: convert io_bus to SRCU") > Cc: stable@vger.kernel.org # 3.4+ > Reported-by: Dmitry Vyukov > Signed-off-by: David Hildenbrand > /* Caller must hold slots_lock. */ > -int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > - struct kvm_io_device *dev) > +void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > + struct kvm_io_device *dev) > { > - int i, r; > + int i; > struct kvm_io_bus *new_bus, *bus; > > bus = kvm->buses[bus_idx]; > - > - /* > - * It's possible the bus being released before hand. If so, > - * we're done here. > - */ > if (!bus) > - return 0; > + return; > > - r = -ENOENT; > for (i = 0; i < bus->dev_count; i++) > if (bus->range[i].dev == dev) { > - r = 0; > break; > } > > - if (r) > - return r; > + if (i == bus->dev_count) > + return; > > new_bus = kmalloc(sizeof(*bus) + ((bus->dev_count - 1) * > sizeof(struct kvm_io_range)), GFP_KERNEL); > - if (!new_bus) > - return -ENOMEM; > + if (!new_bus) { > + pr_err("kvm: failed to shrink bus, removing it completely\n"); > + goto broken; > + } > > memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > new_bus->dev_count--; > memcpy(new_bus->range + i, bus->range + i + 1, > (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > > +broken: > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); As this may set kvm->buses[bus_idx] to NULL, don't you also need to guard for bus == NULL in kvm_io_bus_destroy()? (I looked at the code on kvm/queue.) > synchronize_srcu_expedited(&kvm->srcu); > kfree(bus); > - return r; > + return; > }