[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <_vGVoFJcOuoIAvGYtkyemUvqEFeZ-AdO4Jk8wsyVv3MwO-6NEVtULxnZzuBJNeHNkCsQ5Kxn5TPQ_VJ6qyj9wXXXX8v-hc3HptnCAu0UYsk=@protonmail.com>
Date: Sat, 25 Jul 2020 03:03:52 +0000
From: Mazin Rezk <mnrzk@...tonmail.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org,
amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
Andrew Morton <akpm@...ux-foundation.org>,
Christian König <christian.koenig@....com>,
Harry Wentland <Harry.Wentland@....com>,
Nicholas Kazlauskas <nicholas.kazlauskas@....com>,
sunpeng.li@....com, Alexander Deucher <Alexander.Deucher@....com>,
1i5t5.duncan@....net, mphantomx@...oo.com.br,
regressions@...mhuis.info, anthony.ruhier@...il.com
Subject: Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
On Friday, July 24, 2020 5:19 PM, Paul Menzel <pmenzel@...gen.mpg.de> wrote:
> Dear Kees,
>
> Am 24.07.20 um 19:33 schrieb Kees Cook:
>
> > On Fri, Jul 24, 2020 at 09:45:18AM +0200, Paul Menzel wrote:
> >
> > > Am 24.07.20 um 00:32 schrieb Kees Cook:
> > >
> > > > On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote:
> > > > As Linux 5.8-rc7 is going to be released this Sunday, I wonder, if commit
> > > > 3202fa62f ("slub: relocate freelist pointer to middle of object") should be
> > > > reverted for now to fix the regression for the users according to Linux’ no
> > > > regression policy. Once the AMDGPU/DRM driver issue is fixed, it can be
> > > > reapplied. I know it’s not optimal, but as some testing is going to be
> > > > involved for the fix, I’d argue it’s the best option for the users.
> >
> > Well, the SLUB defense was already released in v5.7, so I'm not sure it
> > really helps for amdgpu_dm users seeing it there too.
>
> In my opinion, it would help, as the stable release could pick up the
> revert, ones it’s in Linus’ master branch.
>
> > There was a fix to disable the async path for this driver that worked
> > around the bug too, yes? That seems like a safer and more focused
> > change that doesn't revert the SLUB defense for all users, and would
> > actually provide a complete, I think, workaround whereas reverting
> > the SLUB change means the race still exists. For example, it would be
> > hit with slab poisoning, etc.
>
> I do not know. If there is such a fix, that would be great. But if you
> do not know, how should a normal user? ;-)
>
> Kind regards,
>
> Paul
>
> Kind regards,
>
> Paul
If we're talking about workarounds now, I suggest simply swapping the base
and context variables in struct dm_atomic_state. By that way, we won't need
to change non-amdgpu parts of the code (e.g. by reverting the SLUB patch).
Prior to 3202fa62f, the freelist pointer was stored in dm_state->base which
was never dereferenced and therefore caused no noticeable issue. After
3202fa62f, the freelist pointer is stored in the middle of the struct (i.e.
dm_state->context).
Swapping the position of the base and context variables in dm_atomic_state
should, in theory, revert this code back to it's pre-5.7 state since the
code would be back to overwriting base instead.
If we decide to use this workaround, I can write the patch and do more
extended tests to confirm it works around the issues.
That said, I haven't seen the async disabling patch. If you could link to
it, I'd be glad to test it out and perhaps we can use that instead.
Thanks,
Mazin Rezk
Powered by blists - more mailing lists