Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 29 Jan 2019 21:36:22 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Lyude Paul <lyude@redhat.com>
Cc:     intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] drm/dp_mst: Fix unbalanced malloc ref in
 drm_dp_mst_deallocate_vcpi()
Message-ID: <20190129203622.GB3271@phenom.ffwll.local>
Mail-Followup-To: Lyude Paul <lyude@redhat.com>,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        linux-kernel@vger.kernel.org
References: <20190129183928.26779-1-lyude@redhat.com>
 <20190129183928.26779-2-lyude@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190129183928.26779-2-lyude@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 29, 2019 at 01:39:25PM -0500, Lyude Paul wrote:
> In drm_dp_mst_deallocate_vcpi(), we currently unconditionally call
> drm_dp_mst_put_port_malloc() on the port that's passed to us, even if we
> never successfully allocated VCPI to it. This is contrary to what we do
> in drm_dp_mst_allocate_vcpi(), where we only call
> drm_dp_mst_get_port_malloc() on the passed port if we successfully
> allocated VCPI to it.
> 
> As a result, if drm_dp_mst_allocate_vcpi() fails during a modeset and
> another successive modeset calls drm_dp_mst_deallocate_vcpi() we will
> end up dropping someone else's malloc reference to the port. Example:
> 
> [  962.309260] ==================================================================
> [  962.309290] BUG: KASAN: use-after-free in drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309296] Read of size 4 at addr ffff888416c30004 by task kworker/0:1H/500
> 
> [  962.309308] CPU: 0 PID: 500 Comm: kworker/0:1H Tainted: G        W  O      5.0.0-rc2Lyude-Test+ #1
> [  962.309313] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018
> [  962.309428] Workqueue: events_highpri intel_atomic_cleanup_work [i915]
> [  962.309434] Call Trace:
> [  962.309452]  dump_stack+0xad/0x150
> [  962.309462]  ? dump_stack_print_info.cold.0+0x1b/0x1b
> [  962.309472]  ? kmsg_dump_rewind_nolock+0xd9/0xd9
> [  962.309504]  ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309515]  print_address_description+0x6c/0x23c
> [  962.309542]  ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309568]  ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309577]  kasan_report.cold.3+0x1a/0x32
> [  962.309605]  ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309631]  drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper]
> [  962.309658]  ? drm_dp_mst_put_mstb_malloc+0x180/0x180 [drm_kms_helper]
> [  962.309687]  drm_dp_mst_destroy_state+0xcd/0x120 [drm_kms_helper]
> [  962.309745]  drm_atomic_state_default_clear+0x6ee/0xcc0 [drm]
> [  962.309864]  intel_atomic_state_clear+0xe/0x80 [i915]
> [  962.309928]  __drm_atomic_state_free+0x35/0xd0 [drm]
> [  962.310044]  intel_atomic_cleanup_work+0x56/0x70 [i915]
> [  962.310057]  process_one_work+0x884/0x1400
> [  962.310067]  ? drain_workqueue+0x5a0/0x5a0
> [  962.310075]  ? __schedule+0x87f/0x1e80
> [  962.310086]  ? __sched_text_start+0x8/0x8
> [  962.310095]  ? run_rebalance_domains+0x400/0x400
> [  962.310110]  ? deref_stack_reg+0xb4/0x120
> [  962.310117]  ? __read_once_size_nocheck.constprop.7+0x10/0x10
> [  962.310124]  ? worker_enter_idle+0x47f/0x6a0
> [  962.310134]  ? schedule+0xd7/0x2e0
> [  962.310141]  ? __schedule+0x1e80/0x1e80
> [  962.310148]  ? _raw_spin_lock_irq+0x9f/0x130
> [  962.310155]  ? _raw_write_unlock_irqrestore+0x110/0x110
> [  962.310164]  worker_thread+0x196/0x11e0
> [  962.310175]  ? set_load_weight+0x2e0/0x2e0
> [  962.310181]  ? __switch_to_asm+0x34/0x70
> [  962.310187]  ? __switch_to_asm+0x40/0x70
> [  962.310194]  ? process_one_work+0x1400/0x1400
> [  962.310199]  ? __switch_to_asm+0x40/0x70
> [  962.310205]  ? __switch_to_asm+0x34/0x70
> [  962.310211]  ? __switch_to_asm+0x34/0x70
> [  962.310216]  ? __switch_to_asm+0x40/0x70
> [  962.310221]  ? __switch_to_asm+0x34/0x70
> [  962.310226]  ? __switch_to_asm+0x40/0x70
> [  962.310231]  ? __switch_to_asm+0x34/0x70
> [  962.310236]  ? __switch_to_asm+0x40/0x70
> [  962.310242]  ? syscall_return_via_sysret+0xf/0x7f
> [  962.310248]  ? __switch_to_asm+0x34/0x70
> [  962.310253]  ? __switch_to_asm+0x40/0x70
> [  962.310258]  ? __switch_to_asm+0x34/0x70
> [  962.310263]  ? __switch_to_asm+0x40/0x70
> [  962.310268]  ? __switch_to_asm+0x34/0x70
> [  962.310273]  ? __switch_to_asm+0x40/0x70
> [  962.310281]  ? __schedule+0x87f/0x1e80
> [  962.310292]  ? __sched_text_start+0x8/0x8
> [  962.310300]  ? save_stack+0x8c/0xb0
> [  962.310308]  ? __kasan_kmalloc.constprop.6+0xc6/0xd0
> [  962.310313]  ? kthread+0x98/0x3a0
> [  962.310318]  ? ret_from_fork+0x35/0x40
> [  962.310334]  ? __wake_up_common+0x178/0x6f0
> [  962.310343]  ? _raw_spin_lock_irqsave+0xa4/0x140
> [  962.310349]  ? __lock_text_start+0x8/0x8
> [  962.310355]  ? _raw_write_lock_irqsave+0x70/0x130
> [  962.310360]  ? __lock_text_start+0x8/0x8
> [  962.310371]  ? process_one_work+0x1400/0x1400
> [  962.310376]  kthread+0x2e2/0x3a0
> [  962.310383]  ? kthread_create_on_node+0xc0/0xc0
> [  962.310389]  ret_from_fork+0x35/0x40
> 
> [  962.310401] Allocated by task 1462:
> [  962.310410]  __kasan_kmalloc.constprop.6+0xc6/0xd0
> [  962.310437]  drm_dp_add_port+0xd60/0x1960 [drm_kms_helper]
> [  962.310464]  drm_dp_send_link_address+0x4b0/0x770 [drm_kms_helper]
> [  962.310491]  drm_dp_check_and_send_link_address+0x197/0x1f0 [drm_kms_helper]
> [  962.310515]  drm_dp_mst_link_probe_work+0x2b6/0x330 [drm_kms_helper]
> [  962.310522]  process_one_work+0x884/0x1400
> [  962.310529]  worker_thread+0x196/0x11e0
> [  962.310533]  kthread+0x2e2/0x3a0
> [  962.310538]  ret_from_fork+0x35/0x40
> 
> [  962.310543] Freed by task 500:
> [  962.310550]  __kasan_slab_free+0x133/0x180
> [  962.310555]  kfree+0x92/0x1a0
> [  962.310581]  drm_dp_mst_put_port_malloc+0x14d/0x180 [drm_kms_helper]
> [  962.310693]  intel_connector_destroy+0xb2/0xe0 [i915]
> [  962.310747]  drm_mode_object_put.part.0+0x12b/0x1a0 [drm]
> [  962.310802]  drm_atomic_state_default_clear+0x1f2/0xcc0 [drm]
> [  962.310916]  intel_atomic_state_clear+0xe/0x80 [i915]
> [  962.310972]  __drm_atomic_state_free+0x35/0xd0 [drm]
> [  962.311083]  intel_atomic_cleanup_work+0x56/0x70 [i915]
> [  962.311092]  process_one_work+0x884/0x1400
> [  962.311098]  worker_thread+0x196/0x11e0
> [  962.311103]  kthread+0x2e2/0x3a0
> [  962.311108]  ret_from_fork+0x35/0x40
> 
> [  962.311116] The buggy address belongs to the object at ffff888416c30000
>                 which belongs to the cache kmalloc-2k of size 2048
> [  962.311122] The buggy address is located 4 bytes inside of
>                 2048-byte region [ffff888416c30000, ffff888416c30800)
> [  962.311124] The buggy address belongs to the page:
> [  962.311132] page:ffffea00105b0c00 count:1 mapcount:0 mapping:ffff88841d003040 index:0x0 compound_mapcount: 0
> [  962.311142] flags: 0x8000000000010200(slab|head)
> [  962.311152] raw: 8000000000010200 dead000000000100 dead000000000200 ffff88841d003040
> [  962.311159] raw: 0000000000000000 00000000000f000f 00000001ffffffff 0000000000000000
> [  962.311162] page dumped because: kasan: bad access detected
> 
> So, bail early if drm_dp_mst_deallocate_vcpi() is called on a port with
> no VCPI allocation. Additionally, clean up the surrounding kerneldoc
> while we're at it since the port is assumed to be kept around because
> the DRM driver is expected to hold a malloc reference to it, not just
> us.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations")
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2552a27362a0..8ba0e5b368da 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3246,15 +3246,13 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots);
>  /**
>   * drm_dp_mst_deallocate_vcpi() - deallocate a VCPI
>   * @mgr: manager for this port
> - * @port: unverified port to deallocate vcpi for
> + * @port: port to deallocate vcpi for

Maybe add:

"This can be called unconditionally, whether drm_dp_mst_allocate_vcpi
succeeded or not."

Just to clarify this a bit more.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>   */
>  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  				struct drm_dp_mst_port *port)
>  {
> -	/*
> -	 * A port with VCPI will remain allocated until it's VCPI is
> -	 * released, no verified ref needed
> -	 */
> +	if (!port->vcpi.vcpi)
> +		return;
>  
>  	drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi);
>  	port->vcpi.num_slots = 0;
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch