EDAC, ghes: Fix use after free and add reference

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/[email protected]

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 unregistered, and start rebinding them again.

Thanks,

James Morse (2):
EDAC, ghes: Fix Use after free in ghes_edac remove path EDAC, ghes: Reference count GHES users of ghes_edac

Quoted 16 lines · James Morse

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/[email protected]

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().

Quoted 3 lines

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?

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...

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.

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.

Provided unregister cannot be called concurrently, the

        if (!ghes_pvt)
                return;

in ghes_edac_unregister() should suffice.

But just to be on the safe side, it could get an "instance_mutex" or so under which ghes_pvt is set and cleared and then that silly counter can simply go away.

Thoughts?

Quoted 5 lines · Borislav Petkov

Provided unregister cannot be called concurrently, the

        if (!ghes_pvt)
                return;

Quoted 7 lines

in ghes_edac_unregister() should suffice.

But just to be on the safe side, it could get an "instance_mutex" or so under which ghes_pvt is set and cleared and then that silly counter can simply go away.

Thoughts?

IOW, something simple and straight-forward like this:

---

drivers/edac/ghes_edac.c +10 -5

index 0bb62857ffb2..b600f010fa0e 100644
@@ -26,9 +26,11 @@ struct ghes_edac_pvt {
 	char msg[80];
 };
 
-static atomic_t ghes_init = ATOMIC_INIT(0);
 static struct ghes_edac_pvt *ghes_pvt;
 
+/* GHES instances reg/dereg mutex */
+static DEFINE_MUTEX(ghes_reg_mutex);
+
 /*
  * Sync with other, potentially concurrent callers of
  * ghes_edac_report_mem_error(). We don't know what the

@@ -461,7 +463,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_dimm_fill dimm_fill;
-	int idx = -1;
+	int idx = -1, err = 0;
 
 	if (IS_ENABLED(CONFIG_X86)) {
 		/* Check if safe to enable on this system */

@@ -472,11 +474,13 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		idx = 0;
 	}
 
+	mutex_lock(&ghes_reg_mutex);
+
 	/*
 	 * We have only one logical memory controller to which all DIMMs belong.
 	 */
-	if (atomic_inc_return(&ghes_init) > 1)
-		return 0;
+	if (ghes_pvt)
+		goto unlock;
 
 	/* Get the number of DIMMs */
 	dmi_walk(ghes_edac_count_dimms, &num_dimm);

@@ -494,7 +498,8 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
 	if (!mci) {
 		pr_info("Can't allocate memory for EDAC data\n");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto unlock;
 	}
 
 	ghes_pvt	= mci->pvt_info;

@@ -541,23 +546,29 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	if (rc < 0) {
 		pr_info("Can't register at EDAC core\n");
 		edac_mc_free(mci);

There are a few more warts we should try and get rid of with this:
ghes_edac_register() publishes the ghes_pvt pointer under the mutex, but the irq handler reads it without taking the mutex. (obviously it can't).

ghes_edac_register() publishes the pointer before its called edac_mc_add_mc(), which is pleasant.

Yeah, before we do this, lemme try to simplify the situation more. And yeah, we don't need the mutex - we can use the spinlock only. But let's get rid of the need to access the pvt in the IRQ handler. Yeah, we need the *mci pointer but one can't have it all :)

Anyway, here's what I have, it is only build-tested. I wanna give it a stern look tomorrow, on a clear head again:

Quoted 7 lines · Borislav Petkov

Yeah, before we do this, lemme try to simplify the situation more. And yeah, we don't need the mutex - we can use the spinlock only. But let's get rid of the need to access the pvt in the IRQ handler. Yeah, we need the *mci pointer but one can't have it all :)

Anyway, here's what I have, it is only build-tested. I wanna give it a stern look tomorrow, on a clear head again:

And now I went and redid your patch ontop and thus completely got rid of ghes_pvt in favor of having one global ghes_mci pointer.

We access it only under the lock and we publish it in ghes_edac_register() only in the success case. Can't get any simpler than that.

Thoughts?

I think it is a lot cleaner and straight-forward this way. Lemme know if you wanna run it on your setup and I'll push the two patches somewhere.

Hi Boris,

Quoted 5 lines · Borislav Petkov

Quoted 4 lines · Borislav Petkov

Provided unregister cannot be called concurrently, the

        if (!ghes_pvt)
                return;

Quoted 69 lines

Quoted 8 lines

in ghes_edac_unregister() should suffice.

But just to be on the safe side, it could get an "instance_mutex" or so under which ghes_pvt is set and cleared and then that silly counter can simply go away.

Thoughts?

IOW, something simple and straight-forward like this:

---

drivers/edac/ghes_edac.c +10 -5

index 0bb62857ffb2..b600f010fa0e 100644
@@ -26,9 +26,11 @@ struct ghes_edac_pvt {
 	char msg[80];
 };
 
-static atomic_t ghes_init = ATOMIC_INIT(0);
 static struct ghes_edac_pvt *ghes_pvt;
 
+/* GHES instances reg/dereg mutex */
+static DEFINE_MUTEX(ghes_reg_mutex);
+
 /*
  * Sync with other, potentially concurrent callers of
  * ghes_edac_report_mem_error(). We don't know what the

@@ -461,7 +463,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_dimm_fill dimm_fill;
-	int idx = -1;
+	int idx = -1, err = 0;
 
 	if (IS_ENABLED(CONFIG_X86)) {
 		/* Check if safe to enable on this system */

@@ -472,11 +474,13 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		idx = 0;
 	}
 
+	mutex_lock(&ghes_reg_mutex);
+
 	/*
 	 * We have only one logical memory controller to which all DIMMs belong.
 	 */
-	if (atomic_inc_return(&ghes_init) > 1)
-		return 0;
+	if (ghes_pvt)
+		goto unlock;
 
 	/* Get the number of DIMMs */
 	dmi_walk(ghes_edac_count_dimms, &num_dimm);

@@ -494,7 +498,8 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
 	if (!mci) {
 		pr_info("Can't allocate memory for EDAC data\n");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto unlock;
 	}
 
 	ghes_pvt	= mci->pvt_info;

@@ -541,23 +546,29 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	if (rc < 0) {
 		pr_info("Can't register at EDAC core\n");
 		edac_mc_free(mci);

drivers/edac/ghes_edac.c int ghes_edac_register(struct ghes *ghes, struct device *dev) · line 556

Quoted 10 lines

drivers/edac/ghes_edac.c +6 -2

-		return -ENODEV;
+		err = -ENODEV;
 	}
-	return 0;
+
+unlock:
+	mutex_unlock(&ghes_reg_mutex);
+
+	return err;
 }

There are a few more warts we should try and get rid of with this:
ghes_edac_register() publishes the ghes_pvt pointer under the mutex, but the irq handler reads it without taking the mutex. (obviously it can't).

ghes_edac_register() publishes the pointer before its called edac_mc_add_mc(), which is pleasant.

(sorry I've been sitting on this while I found new and exciting ways to break my test machine!)

Combined with the spinlocky bits of:
--------------------------%<--------------------------

Quoted 1 line

From 61fa061790fe7c19af25b25693b61bb75a498058 Mon Sep 17 00:00:00 2001

From: James Morse <[email protected]>
Date: Wed, 16 Oct 2019 10:02:15 +0100
Subject: [PATCH] EDAC, ghes: Move ghes_init and ghes_pvt under the ghes_lock

ghes_edac has an irqsave spinlock that protects the contents of ghes_pvt, but not the pointer itself. The register, unregister and irq-handler functions read this bare global variable expecting to see NULL or the allocated value. Without READ_ONCE()/WRITE_ONCE(), this is racy. ghes_edac_register() also publishes the pointer before it has registered the mci.

Replace ghes_init with an unsigned int counter in ghes_pvt. To access this or read the ghes_pvt pointer, you must hold the ghes_lock. This prevents races when these values are modified.

Signed-off-by James Morse <[email protected]>

---
 drivers/edac/ghes_edac.c | 69 ++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 21 deletions(-)

drivers/edac/ghes_edac.c +5 -7

index 0bb62857ffb2..804bb07c6acf 100644
@@ -15,7 +15,10 @@
 #include "edac_module.h"
 #include <ras/ras_event.h>
 
+/* Hold ghes_lock when accessing ghes_pvt */
 struct ghes_edac_pvt {
+	unsigned int users;
+
 	struct list_head list;
 	struct ghes *ghes;
 	struct mem_ctl_info *mci;

@@ -26,7 +29,6 @@ struct ghes_edac_pvt {
 	char msg[80];
 };
 
-static atomic_t ghes_init = ATOMIC_INIT(0);
 static struct ghes_edac_pvt *ghes_pvt;
 
 /*

@@ -79,9 +81,8 @@ static void ghes_edac_count_dimms(const struct dmi_header *dh, void *arg)
 		(*num_dimm)++;
 }
 
-static int get_dimm_smbios_index(u16 handle)
+static int get_dimm_smbios_index(u16 handle, struct mem_ctl_info *mci)
 {
-	struct mem_ctl_info *mci = ghes_pvt->mci;
 	int i;
 
 	for (i = 0; i < mci->tot_dimms; i++) {

@@ -197,15 +198,12 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err
 *mem_err)
 {
 	enum hw_event_mc_err_type type;
 	struct edac_raw_error_desc *e;
+	struct ghes_edac_pvt *pvt;
 	struct mem_ctl_info *mci;
-	struct ghes_edac_pvt *pvt = ghes_pvt;
 	unsigned long flags;
 	char *p;
 	u8 grain_bits;
 
-	if (!pvt)
-		return;
-
 	/*
 	 * We can do the locking below because GHES defers error processing

drivers/edac/ghes_edac.c +5 -0

 	 * from NMI to IRQ context. Whenever that changes, we'd at least

@@ -215,6 +213,11 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err
 *mem_err)
 		return;
 
 	spin_lock_irqsave(&ghes_lock, flags);
+	pvt = ghes_pvt;
+	if (!pvt) {
+		spin_unlock_irqrestore(&ghes_lock, flags);
+		return;
+	}
 
 	mci = pvt->mci;

drivers/edac/ghes_edac.c +3 -1

 	e = &mci->error_desc;

@@ -348,7 +351,7 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 			p += sprintf(p, "DIMM DMI handle: 0x%.4x ",
 				     mem_err->mem_dev_handle);
 
-		index = get_dimm_smbios_index(mem_err->mem_dev_handle);
+		index = get_dimm_smbios_index(mem_err->mem_dev_handle, mci);
 		if (index >= 0) {
 			e->top_layer = index;
 			e->enable_per_layer_report = true;

@@ -457,8 +460,10 @@ static struct acpi_platform_list plat_list[] = {
 int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
 	bool fake = false;
+	unsigned long flags;
 	int rc, num_dimm = 0;
 	struct mem_ctl_info *mci;
+	struct ghes_edac_pvt *pvt;
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_dimm_fill dimm_fill;
 	int idx = -1;

drivers/edac/ghes_edac.c +7 -3

+	spin_unlock_irqrestore(&ghes_lock, flags);
+
+	if (pvt)
 		return 0;
 
 	/* Get the number of DIMMs */

@@ -497,9 +508,10 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 		return -ENOMEM;
 	}
 
-	ghes_pvt	= mci->pvt_info;
-	ghes_pvt->ghes	= ghes;
-	ghes_pvt->mci	= mci;
+	pvt		= mci->pvt_info;
+	pvt->ghes	= ghes;
+	pvt->mci	= mci;
+	pvt->users	= 1;
 
 	mci->pdev = dev;
 	mci->mtype_cap = MEM_FLAG_EMPTY;

drivers/edac/ghes_edac.c +14 -5

+
 	return 0;
 }
 
 void ghes_edac_unregister(struct ghes *ghes)
 {
 	struct mem_ctl_info *mci;
+	bool do_free = false;
+	unsigned long flags;
 
-	if (!ghes_pvt)
-		return;
-
-	if (atomic_dec_return(&ghes_init))
-		return;
+	spin_lock_irqsave(&ghes_lock, flags);
+	if (ghes_pvt) {
+		ghes_pvt->users -= 1;
+
+		/* Are we the last user? */
+		if (!ghes_pvt->users) {
+			do_free = true;
+			mci = ghes_pvt->mci;
+			ghes_pvt = NULL;
+		}
+	}

drivers/edac/ghes_edac.c +0 -0

 }

Signature · 8 lines

2.20.1
--------------------------%<--------------------------



Thanks,

James

Hello,

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/[email protected]

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 unregistered, and start rebinding them again.

Thanks,

James Morse (2):
EDAC, ghes: Fix Use after free in ghes_edac remove path EDAC, ghes: Reference count GHES users of ghes_edac

 drivers/edac/ghes_edac.c | 4 ++++
 1 file changed, 4 insertions(+)