From owner-svn-src-stable@freebsd.org Mon Jul 3 15:34:22 2017 Return-Path: Delivered-To: svn-src-stable@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5AE879E8FF4; Mon, 3 Jul 2017 15:34:22 +0000 (UTC) (envelope-from ken@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 346C074774; Mon, 3 Jul 2017 15:34:22 +0000 (UTC) (envelope-from ken@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v63FYLYm065683; Mon, 3 Jul 2017 15:34:21 GMT (envelope-from ken@FreeBSD.org) Received: (from ken@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v63FYLIk065682; Mon, 3 Jul 2017 15:34:21 GMT (envelope-from ken@FreeBSD.org) Message-Id: <201707031534.v63FYLIk065682@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: ken set sender to ken@FreeBSD.org using -f From: "Kenneth D. Merry" Date: Mon, 3 Jul 2017 15:34:21 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r320602 - stable/11/sys/cam X-SVN-Group: stable-11 X-SVN-Commit-Author: ken X-SVN-Commit-Paths: stable/11/sys/cam X-SVN-Commit-Revision: 320602 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jul 2017 15:34:22 -0000 Author: ken Date: Mon Jul 3 15:34:21 2017 New Revision: 320602 URL: https://svnweb.freebsd.org/changeset/base/320602 Log: MFC r320421: ------------------------------------------------------------------------ r320421 | ken | 2017-06-27 13:26:02 -0600 (Tue, 27 Jun 2017) | 37 lines Fix a panic in camperiphfree(). If a peripheral driver (e.g. da, sa, cd) is added or removed from the peripheral driver list while an unrelated peripheral driver instance (e.g. da0, sa5, cd2) is going away and is inside camperiphfree(), we could dereference an invalid pointer. When peripheral drivers are added or removed (see periphdriver_register() and periphdriver_unregister()), the peripheral driver array is resized and existing entries are moved. Although we hold the topology lock while we traverse the peripheral driver list, we retain a pointer to the location of the peripheral driver pointer and then drop the topology lock. So we are still vulnerable to the list getting moved around while the lock is dropped. To solve the problem, cache a copy of the peripheral driver pointer. If its storage location in the list changes while we have the lock dropped, it won't have any effect. This doesn't solve the issue that peripheral drivers ("da", "cd", as opposed to individual instances like "da0", "cd0") are not generally part of a reference counting scheme to guard against deregistering them while there are instances active. The caller (generally the person unloading a module) has to be aware of active drivers and not unload something that is in use. sys/cam/cam_periph.c: In camperiphfree(), cache a pointer to the peripheral driver instance to avoid holding a pointer to an invalid memory location in the event that the peripheral driver list changes while we have the topology lock dropped. PR: kern/219701 Submitted by: avg Sponsored by: Spectra Logic ------------------------------------------------------------------------ PR: kern/219701 Sponsored by: Spectra Logic Modified: stable/11/sys/cam/cam_periph.c Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/cam/cam_periph.c ============================================================================== --- stable/11/sys/cam/cam_periph.c Mon Jul 3 15:34:19 2017 (r320601) +++ stable/11/sys/cam/cam_periph.c Mon Jul 3 15:34:21 2017 (r320602) @@ -654,6 +654,7 @@ static void camperiphfree(struct cam_periph *periph) { struct periph_driver **p_drv; + struct periph_driver *drv; cam_periph_assert(periph, MA_OWNED); KASSERT(periph->periph_allocating == 0, ("%s%d: freed while allocating", @@ -666,6 +667,15 @@ camperiphfree(struct cam_periph *periph) printf("camperiphfree: attempt to free non-existant periph\n"); return; } + /* + * Cache a pointer to the periph_driver structure. If a + * periph_driver is added or removed from the array (see + * periphdriver_register()) while we drop the toplogy lock + * below, p_drv may change. This doesn't protect against this + * particular periph_driver going away. That will require full + * reference counting in the periph_driver infrastructure. + */ + drv = *p_drv; /* * We need to set this flag before dropping the topology lock, to @@ -701,8 +711,8 @@ camperiphfree(struct cam_periph *periph) */ xpt_lock_buses(); - TAILQ_REMOVE(&(*p_drv)->units, periph, unit_links); - (*p_drv)->generation++; + TAILQ_REMOVE(&drv->units, periph, unit_links); + drv->generation++; xpt_remove_periph(periph);