linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: James Morse <james.morse@arm.com>
To: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Tony Luck <tony.luck@intel.com>,
	Robert Richter <rrichter@marvell.com>,
	John Garry <john.garry@huawei.com>
Subject: Re: [PATCH 0/2] EDAC, ghes: Fix use after free and add reference
Date: Mon, 14 Oct 2019 18:40:33 +0100	[thread overview]
Message-ID: <86ba3fcf-d29c-1d6a-d8c3-2a03cb11263e@arm.com> (raw)
In-Reply-To: <20191014173006.GG4715@zn.tnic>

Hi Boris,

On 14/10/2019 18:30, Borislav Petkov wrote:
> On Mon, Oct 14, 2019 at 06:19:17PM +0100, James Morse wrote:
>> ghes_edac can only be registered once, later attempts will silently
>> do nothing as the driver is already setup. The unregister path also
>> only expects to be called once, but doesn't check.
>>
>> This leads to KASAN splats if multiple GHES entries are unregistered,
>> as the free()d memory is dereferenced, and if we're lucky, free()d
>> a second time.
>>
>> Link: lore.kernel.org/r/304df85b-8b56-b77e-1a11-aa23769f2e7c@huawei.com
>>
>> Patch 1 is the minimum needed to prevent the dereference and double
>> free, but this does expose the lack of symmetry. If we unregister
>> one GHES entry, subsequent notifications will be lost.
>> Unregistering is unsafe if another CPU is using the free()d memory in
>> ghes_edac_report_mem_error().
>>
>> To fix this, Patch 2 uses ghes_init as a reference count.
>>
>> We can now unbind all the GHES entries, causing ghes_edac to be

> Thanks for debugging and fixing this but why two patches?
> 
> One is perfectly fine IMO. Or?

They can be merged together if you prefer.

Keeping both avoids the !pvt check in ghes_edac_report_mem_error() going wrong, but I'm
not entirely sure what that is trying to stop...

The possibility of races between notifications and unregister only occurred to me after
testing the first patch, so they ended up as different things in my head: I thought it
deserved its own commit log as its unrelated to the KASAN splat.


> Btw, I admit that the ghes_init thing is ugly as hell. ;-\

> I wonder if it could be ripped out completely and we use only ghes_pvt
> for controlling the single driver instance loading and unloading...

I think you need some kind of reference count to know how many more GHES.x there are out
there that may call unregister, otherwise you race with notifications.


Thanks,

James

  reply	other threads:[~2019-10-14 17:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-14 17:19 [PATCH 0/2] EDAC, ghes: Fix use after free and add reference James Morse
2019-10-14 17:19 ` [PATCH 1/2] EDAC, ghes: Fix Use after free in ghes_edac remove path James Morse
2019-10-14 17:19 ` [PATCH 2/2] EDAC, ghes: Reference count GHES users of ghes_edac James Morse
2019-10-14 17:30 ` [PATCH 0/2] EDAC, ghes: Fix use after free and add reference Borislav Petkov
2019-10-14 17:40   ` James Morse [this message]
2019-10-14 17:53     ` Borislav Petkov
2019-10-16 15:17       ` Borislav Petkov
2019-10-16 15:30         ` James Morse
2019-10-16 18:50           ` Borislav Petkov
2019-10-21  7:37             ` Borislav Petkov
2019-10-21 10:52               ` Robert Richter
2019-10-22 13:25           ` Robert Richter
2019-10-15 13:25 ` Borislav Petkov

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=86ba3fcf-d29c-1d6a-d8c3-2a03cb11263e@arm.com \
    --to=james.morse@arm.com \
    --cc=bp@alien8.de \
    --cc=john.garry@huawei.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=rrichter@marvell.com \
    --cc=tony.luck@intel.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).