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
| ||
|
Date: Mon, 27 Jul 2020 10:05:01 -0400 From: "Kazlauskas, Nicholas" <nicholas.kazlauskas@....com> To: Christian König <christian.koenig@....com>, Mazin Rezk <mnrzk@...tonmail.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>, "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org> Cc: Andrew Morton <akpm@...ux-foundation.org>, Harry Wentland <Harry.Wentland@....com>, "sunpeng.li@....com" <sunpeng.li@....com>, Kees Cook <keescook@...omium.org>, Alexander Deucher <Alexander.Deucher@....com>, Duncan <1i5t5.duncan@....net>, "mphantomx@...oo.com.br" <mphantomx@...oo.com.br>, "regressions@...mhuis.info" <regressions@...mhuis.info>, "anthony.ruhier@...il.com" <anthony.ruhier@...il.com>, Paul Menzel <pmenzel@...gen.mpg.de> Subject: Re: [PATCH] drm/amd/display: Clear dm_state for fast updates On 2020-07-27 9:39 a.m., Christian König wrote: > Am 27.07.20 um 07:40 schrieb Mazin Rezk: >> This patch fixes a race condition that causes a use-after-free during >> amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits >> are requested and the second one finishes before the first. Essentially, >> this bug occurs when the following sequence of events happens: >> >> 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is >> deferred to the workqueue. >> >> 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is >> deferred to the workqueue. >> >> 3. Commit #2 starts before commit #1, dm_state #1 is used in the >> commit_tail and commit #2 completes, freeing dm_state #1. >> >> 4. Commit #1 starts after commit #2 completes, uses the freed dm_state >> 1 and dereferences a freelist pointer while setting the context. > > Well I only have a one mile high view on this, but why don't you let the > work items execute in order? > > That would be better anyway cause this way we don't trigger a cache line > ping pong between CPUs. > > Christian. We use the DRM helpers for managing drm_atomic_commit_state and those helpers internally push non-blocking commit work into the system unbound work queue. While we could duplicate a copy of that code with nothing but the workqueue changed that isn't something I'd really like to maintain going forward. Regards, Nicholas Kazlauskas > >> >> Since this bug has only been spotted with fast commits, this patch fixes >> the bug by clearing the dm_state instead of using the old dc_state for >> fast updates. In addition, since dm_state is only used for its dc_state >> and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is >> found, >> removing the dm_state should not have any consequences in fast updates. >> >> This use-after-free bug has existed for a while now, but only caused a >> noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate >> freelist pointer to middle of object") moving the freelist pointer from >> dm_state->base (which was unused) to dm_state->context (which is >> dereferenced). >> >> Bugzilla: >> https://bugzilla.kernel.org/show_bug.cgi?id=207383 >> >> Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for >> fast updates") >> Reported-by: Duncan <1i5t5.duncan@....net> >> Signed-off-by: Mazin Rezk <mnrzk@...tonmail.com> >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++++++++++++++----- >> 1 file changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 86ffa0c2880f..710edc70e37e 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct >> drm_device *dev, >> * the same resource. If we have a new DC context as part of >> * the DM atomic state from validation we need to free it and >> * retain the existing one instead. >> + * >> + * Furthermore, since the DM atomic state only contains the DC >> + * context and can safely be annulled, we can free the state >> + * and clear the associated private object now to free >> + * some memory and avoid a possible use-after-free later. >> */ >> - struct dm_atomic_state *new_dm_state, *old_dm_state; >> >> - new_dm_state = dm_atomic_get_new_state(state); >> - old_dm_state = dm_atomic_get_old_state(state); >> + for (i = 0; i < state->num_private_objs; i++) { >> + struct drm_private_obj *obj = state->private_objs[i].ptr; >> >> - if (new_dm_state && old_dm_state) { >> - if (new_dm_state->context) >> - dc_release_state(new_dm_state->context); >> + if (obj->funcs == adev->dm.atomic_obj.funcs) { >> + int j = state->num_private_objs-1; >> >> - new_dm_state->context = old_dm_state->context; >> + dm_atomic_destroy_state(obj, >> + state->private_objs[i].state); >> + >> + /* If i is not at the end of the array then the >> + * last element needs to be moved to where i was >> + * before the array can safely be truncated. >> + */ >> + if (i != j) >> + state->private_objs[i] = >> + state->private_objs[j]; >> >> - if (old_dm_state->context) >> - dc_retain_state(old_dm_state->context); >> + state->private_objs[j].ptr = NULL; >> + state->private_objs[j].state = NULL; >> + state->private_objs[j].old_state = NULL; >> + state->private_objs[j].new_state = NULL; >> + >> + state->num_private_objs = j; >> + break; >> + } >> } >> } >> >> -- >> 2.27.0 >> >
Powered by blists - more mailing lists