All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Christian König" <christian.koenig@amd.com>
To: Mazin Rezk <mnrzk@protonmail.com>, Kees Cook <keescook@chromium.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"harry.wentland@amd.com" <harry.wentland@amd.com>,
	"nicholas.kazlauskas@amd.com" <nicholas.kazlauskas@amd.com>,
	"sunpeng.li@amd.com" <sunpeng.li@amd.com>,
	"alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	"1i5t5.duncan@cox.net" <1i5t5.duncan@cox.net>,
	"mphantomx@yahoo.com.br" <mphantomx@yahoo.com.br>,
	"regressions@leemhuis.info" <regressions@leemhuis.info>,
	"anthony.ruhier@gmail.com" <anthony.ruhier@gmail.com>,
	"pmenzel@molgen.mpg.de" <pmenzel@molgen.mpg.de>
Subject: Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
Date: Fri, 24 Jul 2020 09:26:32 +0200	[thread overview]
Message-ID: <6ab6f915-b316-97b4-53ab-21a5531637c7@amd.com> (raw)
In-Reply-To: <4KGdosy_NHW6FYCc2rCq4e71vYI7e4InqrLJ4S1GJsLcfjHv_INF-CzIYusOFqznOxyvflBTlFCXvyy7J37fn-QKfNOQK78MM38VdjdUXks=@protonmail.com>

Am 24.07.20 um 00:58 schrieb Mazin Rezk:
> On Thursday, July 23, 2020 6:32 PM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote:
>>
>>> When amdgpu_dm_atomic_commit_tail is running in the workqueue,
>>> drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
>>> running, causing a race condition where state (and then dm_state) is
>>> sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
>>> occurred since 5.7-rc1 and is well documented among polaris11 users [1].
>>> Prior to 5.7, this was not a noticeable issue since the freelist pointer
>>> was stored at the beginning of dm_state (base), which was unused. After
>>> changing the freelist pointer to be stored in the middle of the struct, the
>>> freelist pointer overwrote the context, causing dc_state to become garbage
>>> data and made the call to dm_enable_per_frame_crtc_master_sync dereference
>>> a freelist pointer.
>>> This patch fixes the aforementioned issue by calling drm_atomic_state_get
>>> in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
>>> drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
>>> According to my testing on 5.8.0-rc6, this should fix bug 207383 on
>>> Bugzilla [1].
>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D207383&amp;data=02%7C01%7Charry.wentland%40amd.com%7C53cc9cffb1d244d7b43508d82f5bed1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637311419153032496&amp;sdata=t45vmEJ80UXOmRfndGfe69AOedtkFUwDqvWgGDrSuOk%3D&amp;reserved=0
>> Nice work tracking this down!
>>
>>> Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
>> I do, however, object to this Fixes tag. :) The flaw appears to have
>> been with amdgpu_dm's reference tracking of "state" in the nonblocking
>> case. (How this reference counting is supposed to work correctly, though,
>> I'm not sure.) If I look at where the drm helper was split from being
>> the default callback, it looks like this was what introduced the bug:
>>
>> da5c47f682ab ("drm/amd/display: Remove acrtc->stream")
>>
>> ? 3202fa62f certainly exposed it much more quickly, but there was a race
>> even without 3202fa62f where something could have realloced the memory
>> and written over it.
>>
>> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> Kees Cook
>
> Thanks, I'll be sure to avoid using 3202fa62f as the cause next time.
> I just thought to do that because it was what made the use-after-free cause
> a noticeable bug.
>
> Also, by the way, I just realised the patch didn't completely solve the bug.
> Sorry about that, making an LKML thread on this was hasty on my part. Should
> I get further confirmation from the Bugzilla thread before submitting a patch
> for this bug in the future?

Submitting stuff as early as possible is mostly a good idea. Just if the 
code is utterly broken or completely unreadable you should probably 
expect a harsh response :)

Maybe ask for more testing in the commit message if you are not 100% 
sure if that really fixes a bug or not.

Regards,
Christian.

>
> Thanks,
> Mazin Rezk


WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com>
To: Mazin Rezk <mnrzk@protonmail.com>, Kees Cook <keescook@chromium.org>
Cc: "pmenzel@molgen.mpg.de" <pmenzel@molgen.mpg.de>,
	"anthony.ruhier@gmail.com" <anthony.ruhier@gmail.com>,
	"1i5t5.duncan@cox.net" <1i5t5.duncan@cox.net>,
	"sunpeng.li@amd.com" <sunpeng.li@amd.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"regressions@leemhuis.info" <regressions@leemhuis.info>,
	"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
	"alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"mphantomx@yahoo.com.br" <mphantomx@yahoo.com.br>,
	"nicholas.kazlauskas@amd.com" <nicholas.kazlauskas@amd.com>
Subject: Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
Date: Fri, 24 Jul 2020 09:26:32 +0200	[thread overview]
Message-ID: <6ab6f915-b316-97b4-53ab-21a5531637c7@amd.com> (raw)
In-Reply-To: <4KGdosy_NHW6FYCc2rCq4e71vYI7e4InqrLJ4S1GJsLcfjHv_INF-CzIYusOFqznOxyvflBTlFCXvyy7J37fn-QKfNOQK78MM38VdjdUXks=@protonmail.com>

Am 24.07.20 um 00:58 schrieb Mazin Rezk:
> On Thursday, July 23, 2020 6:32 PM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote:
>>
>>> When amdgpu_dm_atomic_commit_tail is running in the workqueue,
>>> drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
>>> running, causing a race condition where state (and then dm_state) is
>>> sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
>>> occurred since 5.7-rc1 and is well documented among polaris11 users [1].
>>> Prior to 5.7, this was not a noticeable issue since the freelist pointer
>>> was stored at the beginning of dm_state (base), which was unused. After
>>> changing the freelist pointer to be stored in the middle of the struct, the
>>> freelist pointer overwrote the context, causing dc_state to become garbage
>>> data and made the call to dm_enable_per_frame_crtc_master_sync dereference
>>> a freelist pointer.
>>> This patch fixes the aforementioned issue by calling drm_atomic_state_get
>>> in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
>>> drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
>>> According to my testing on 5.8.0-rc6, this should fix bug 207383 on
>>> Bugzilla [1].
>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D207383&amp;data=02%7C01%7Charry.wentland%40amd.com%7C53cc9cffb1d244d7b43508d82f5bed1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637311419153032496&amp;sdata=t45vmEJ80UXOmRfndGfe69AOedtkFUwDqvWgGDrSuOk%3D&amp;reserved=0
>> Nice work tracking this down!
>>
>>> Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
>> I do, however, object to this Fixes tag. :) The flaw appears to have
>> been with amdgpu_dm's reference tracking of "state" in the nonblocking
>> case. (How this reference counting is supposed to work correctly, though,
>> I'm not sure.) If I look at where the drm helper was split from being
>> the default callback, it looks like this was what introduced the bug:
>>
>> da5c47f682ab ("drm/amd/display: Remove acrtc->stream")
>>
>> ? 3202fa62f certainly exposed it much more quickly, but there was a race
>> even without 3202fa62f where something could have realloced the memory
>> and written over it.
>>
>> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> Kees Cook
>
> Thanks, I'll be sure to avoid using 3202fa62f as the cause next time.
> I just thought to do that because it was what made the use-after-free cause
> a noticeable bug.
>
> Also, by the way, I just realised the patch didn't completely solve the bug.
> Sorry about that, making an LKML thread on this was hasty on my part. Should
> I get further confirmation from the Bugzilla thread before submitting a patch
> for this bug in the future?

Submitting stuff as early as possible is mostly a good idea. Just if the 
code is utterly broken or completely unreadable you should probably 
expect a harsh response :)

Maybe ask for more testing in the commit message if you are not 100% 
sure if that really fixes a bug or not.

Regards,
Christian.

>
> Thanks,
> Mazin Rezk

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com>
To: Mazin Rezk <mnrzk@protonmail.com>, Kees Cook <keescook@chromium.org>
Cc: "pmenzel@molgen.mpg.de" <pmenzel@molgen.mpg.de>,
	"anthony.ruhier@gmail.com" <anthony.ruhier@gmail.com>,
	"1i5t5.duncan@cox.net" <1i5t5.duncan@cox.net>,
	"sunpeng.li@amd.com" <sunpeng.li@amd.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"regressions@leemhuis.info" <regressions@leemhuis.info>,
	"amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
	"alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"mphantomx@yahoo.com.br" <mphantomx@yahoo.com.br>,
	"harry.wentland@amd.com" <harry.wentland@amd.com>,
	"nicholas.kazlauskas@amd.com" <nicholas.kazlauskas@amd.com>
Subject: Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
Date: Fri, 24 Jul 2020 09:26:32 +0200	[thread overview]
Message-ID: <6ab6f915-b316-97b4-53ab-21a5531637c7@amd.com> (raw)
In-Reply-To: <4KGdosy_NHW6FYCc2rCq4e71vYI7e4InqrLJ4S1GJsLcfjHv_INF-CzIYusOFqznOxyvflBTlFCXvyy7J37fn-QKfNOQK78MM38VdjdUXks=@protonmail.com>

Am 24.07.20 um 00:58 schrieb Mazin Rezk:
> On Thursday, July 23, 2020 6:32 PM, Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote:
>>
>>> When amdgpu_dm_atomic_commit_tail is running in the workqueue,
>>> drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
>>> running, causing a race condition where state (and then dm_state) is
>>> sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
>>> occurred since 5.7-rc1 and is well documented among polaris11 users [1].
>>> Prior to 5.7, this was not a noticeable issue since the freelist pointer
>>> was stored at the beginning of dm_state (base), which was unused. After
>>> changing the freelist pointer to be stored in the middle of the struct, the
>>> freelist pointer overwrote the context, causing dc_state to become garbage
>>> data and made the call to dm_enable_per_frame_crtc_master_sync dereference
>>> a freelist pointer.
>>> This patch fixes the aforementioned issue by calling drm_atomic_state_get
>>> in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
>>> drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
>>> According to my testing on 5.8.0-rc6, this should fix bug 207383 on
>>> Bugzilla [1].
>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D207383&amp;data=02%7C01%7Charry.wentland%40amd.com%7C53cc9cffb1d244d7b43508d82f5bed1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637311419153032496&amp;sdata=t45vmEJ80UXOmRfndGfe69AOedtkFUwDqvWgGDrSuOk%3D&amp;reserved=0
>> Nice work tracking this down!
>>
>>> Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
>> I do, however, object to this Fixes tag. :) The flaw appears to have
>> been with amdgpu_dm's reference tracking of "state" in the nonblocking
>> case. (How this reference counting is supposed to work correctly, though,
>> I'm not sure.) If I look at where the drm helper was split from being
>> the default callback, it looks like this was what introduced the bug:
>>
>> da5c47f682ab ("drm/amd/display: Remove acrtc->stream")
>>
>> ? 3202fa62f certainly exposed it much more quickly, but there was a race
>> even without 3202fa62f where something could have realloced the memory
>> and written over it.
>>
>> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> Kees Cook
>
> Thanks, I'll be sure to avoid using 3202fa62f as the cause next time.
> I just thought to do that because it was what made the use-after-free cause
> a noticeable bug.
>
> Also, by the way, I just realised the patch didn't completely solve the bug.
> Sorry about that, making an LKML thread on this was hasty on my part. Should
> I get further confirmation from the Bugzilla thread before submitting a patch
> for this bug in the future?

Submitting stuff as early as possible is mostly a good idea. Just if the 
code is utterly broken or completely unreadable you should probably 
expect a harsh response :)

Maybe ask for more testing in the commit message if you are not 100% 
sure if that really fixes a bug or not.

Regards,
Christian.

>
> Thanks,
> Mazin Rezk

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

  reply	other threads:[~2020-07-24  7:26 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23 21:10 [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free Mazin Rezk
2020-07-23 21:10 ` Mazin Rezk
2020-07-23 21:10 ` Mazin Rezk
2020-07-23 22:16 ` Kazlauskas, Nicholas
2020-07-23 22:16   ` Kazlauskas, Nicholas
2020-07-23 22:16   ` Kazlauskas, Nicholas
2020-07-23 22:57   ` Mazin Rezk
2020-07-23 22:57     ` Mazin Rezk
2020-07-23 22:57     ` Mazin Rezk
2020-07-24 21:09     ` Mazin Rezk
2020-07-24 21:09       ` Mazin Rezk
2020-07-24 21:09       ` Mazin Rezk
2020-07-23 22:32 ` Kees Cook
2020-07-23 22:32   ` Kees Cook
2020-07-23 22:32   ` Kees Cook
2020-07-23 22:58   ` Mazin Rezk
2020-07-23 22:58     ` Mazin Rezk
2020-07-23 22:58     ` Mazin Rezk
2020-07-24  7:26     ` Christian König [this message]
2020-07-24  7:26       ` Christian König
2020-07-24  7:26       ` Christian König
2020-07-24  7:45   ` Paul Menzel
2020-07-24  7:45     ` Paul Menzel
2020-07-24  7:45     ` Paul Menzel
2020-07-24 17:33     ` Kees Cook
2020-07-24 17:33       ` Kees Cook
2020-07-24 17:33       ` Kees Cook
2020-07-24 21:19       ` Paul Menzel
2020-07-24 21:19         ` Paul Menzel
2020-07-24 21:19         ` Paul Menzel
2020-07-25  3:03         ` Mazin Rezk
2020-07-25  3:03           ` Mazin Rezk
2020-07-25  3:03           ` Mazin Rezk
2020-07-25  4:59           ` Duncan
2020-07-25  4:59             ` Duncan
2020-07-25  4:59             ` Duncan
2020-07-25  5:20             ` Mazin Rezk
2020-07-25  5:20               ` Mazin Rezk
2020-07-25  5:20               ` Mazin Rezk
2020-07-28  9:22               ` Paul Menzel
2020-07-28  9:22                 ` Paul Menzel
2020-07-28  9:22                 ` Paul Menzel
2020-07-28 17:07                 ` Kazlauskas, Nicholas
2020-07-28 17:07                   ` Kazlauskas, Nicholas
2020-07-28 17:07                   ` Kazlauskas, Nicholas
2020-07-28 21:58                   ` daniel
2020-07-28 21:58                     ` daniel
2020-07-28 21:58                     ` daniel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6ab6f915-b316-97b4-53ab-21a5531637c7@amd.com \
    --to=christian.koenig@amd.com \
    --cc=1i5t5.duncan@cox.net \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.deucher@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=anthony.ruhier@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=harry.wentland@amd.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mnrzk@protonmail.com \
    --cc=mphantomx@yahoo.com.br \
    --cc=nicholas.kazlauskas@amd.com \
    --cc=pmenzel@molgen.mpg.de \
    --cc=regressions@leemhuis.info \
    --cc=sunpeng.li@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.