[PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump

[PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump

[Vasant Hegde]

Every dump reported by OPAL is exported to userspace through a sysfs
interface and notified using kobject_uevent(). The userspace daemon
(opal_errd) then reads the dump and acknowledges that the dump is
saved safely to disk. Once acknowledged the kernel removes the
respective sysfs file entry causing respective resources to be
released including kobject.

However it's possible the userspace daemon may already be scanning
dump entries when a new sysfs dump entry is created by the kernel.
User daemon may read this new entry and ack it even before kernel can
notify userspace about it through kobject_uevent() call. If that
happens then we have a potential race between
dump_ack_store->kobject_put() and kobject_uevent which can lead to
use-after-free of a kernfs object resulting in a kernel crash.

This patch fixes this race by protecting the sysfs file
creation/notification by holding a reference count on kobject until we
safely send kobject_uevent().

The function create_dump_obj() returns the dump object which if used
by caller function will end up in use-after-free problem again.
However, the return value of create_dump_obj() function isn't being
used today and there is no need as well. Hence change it to return
void to make this fix complete.

Fixes: c7e64b9c ("powerpc/powernv Platform dump interface")
CC: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-dump.c | 31 +++++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
index 543c816fa99e..7e6eeedec32b 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -318,15 +318,14 @@ static ssize_t dump_attr_read(struct file *filep, struct kobject *kobj,
 	return count;
 }
 
-static struct dump_obj *create_dump_obj(uint32_t id, size_t size,
-					uint32_t type)
+static void create_dump_obj(uint32_t id, size_t size, uint32_t type)
 {
 	struct dump_obj *dump;
 	int rc;
 
 	dump = kzalloc(sizeof(*dump), GFP_KERNEL);
 	if (!dump)
-		return NULL;
+		return;
 
 	dump->kobj.kset = dump_kset;
 
@@ -346,21 +345,39 @@ static struct dump_obj *create_dump_obj(uint32_t id, size_t size,
 	rc = kobject_add(&dump->kobj, NULL, "0x%x-0x%x", type, id);
 	if (rc) {
 		kobject_put(&dump->kobj);
-		return NULL;
+		return;
 	}
 
+	/*
+	 * As soon as the sysfs file for this dump is created/activated there is
+	 * a chance the opal_errd daemon (or any userspace) might read and
+	 * acknowledge the dump before kobject_uevent() is called. If that
+	 * happens then there is a potential race between
+	 * dump_ack_store->kobject_put() and kobject_uevent() which leads to a
+	 * use-after-free of a kernfs object resulting in a kernel crash.
+	 *
+	 * To avoid that, we need to take a reference on behalf of the bin file,
+	 * so that our reference remains valid while we call kobject_uevent().
+	 * We then drop our reference before exiting the function, leaving the
+	 * bin file to drop the last reference (if it hasn't already).
+	 */
+
+	/* Take a reference for the bin file */
+	kobject_get(&dump->kobj);
 	rc = sysfs_create_bin_file(&dump->kobj, &dump->dump_attr);
 	if (rc) {
 		kobject_put(&dump->kobj);
-		return NULL;
+		/* Drop reference count taken for bin file */
+		kobject_put(&dump->kobj);
+		return;
 	}
 
 	pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
 		__func__, dump->id, dump->size);
 
 	kobject_uevent(&dump->kobj, KOBJ_ADD);
-
-	return dump;
+	/* Drop reference count taken for bin file */
+	kobject_put(&dump->kobj);
 }
 
 static irqreturn_t process_dump(int irq, void *data)
-- 
2.26.2

Re: [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump

[Michael Ellerman]

Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
> index 543c816fa99e..7e6eeedec32b 100644
> --- a/arch/powerpc/platforms/powernv/opal-dump.c
> +++ b/arch/powerpc/platforms/powernv/opal-dump.c
> @@ -346,21 +345,39 @@ static struct dump_obj *create_dump_obj(uint32_t id, size_t size,
>  	rc = kobject_add(&dump->kobj, NULL, "0x%x-0x%x", type, id);
>  	if (rc) {
>  		kobject_put(&dump->kobj);
> -		return NULL;
> +		return;
>  	}
>  
> +	/*
> +	 * As soon as the sysfs file for this dump is created/activated there is
> +	 * a chance the opal_errd daemon (or any userspace) might read and
> +	 * acknowledge the dump before kobject_uevent() is called. If that
> +	 * happens then there is a potential race between
> +	 * dump_ack_store->kobject_put() and kobject_uevent() which leads to a
> +	 * use-after-free of a kernfs object resulting in a kernel crash.
> +	 *
> +	 * To avoid that, we need to take a reference on behalf of the bin file,
> +	 * so that our reference remains valid while we call kobject_uevent().
> +	 * We then drop our reference before exiting the function, leaving the
> +	 * bin file to drop the last reference (if it hasn't already).
> +	 */
> +
> +	/* Take a reference for the bin file */
> +	kobject_get(&dump->kobj);
>  	rc = sysfs_create_bin_file(&dump->kobj, &dump->dump_attr);
>  	if (rc) {
>  		kobject_put(&dump->kobj);
> -		return NULL;
> +		/* Drop reference count taken for bin file */
> +		kobject_put(&dump->kobj);
> +		return;
>  	}
>  
>  	pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
>  		__func__, dump->id, dump->size);
>  
>  	kobject_uevent(&dump->kobj, KOBJ_ADD);
> -
> -	return dump;
> +	/* Drop reference count taken for bin file */
> +	kobject_put(&dump->kobj);
>  }

I think this would be better if it was reworked along the lines of:

aea948bb80b4 ("powerpc/powernv/elog: Fix race while processing OPAL error log event.")

cheers

Re: [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump

[Vasant Hegde]

On 10/9/20 10:36 AM, Michael Ellerman wrote:
> Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
>> diff --git a/arch/powerpc/platforms/powernv/opal-dump.c b/arch/powerpc/platforms/powernv/opal-dump.c
>> index 543c816fa99e..7e6eeedec32b 100644
>> --- a/arch/powerpc/platforms/powernv/opal-dump.c
>> +++ b/arch/powerpc/platforms/powernv/opal-dump.c
>> @@ -346,21 +345,39 @@ static struct dump_obj *create_dump_obj(uint32_t id, size_t size,
>>   	rc = kobject_add(&dump->kobj, NULL, "0x%x-0x%x", type, id);
>>   	if (rc) {
>>   		kobject_put(&dump->kobj);
>> -		return NULL;
>> +		return;
>>   	}
>>   
>> +	/*
>> +	 * As soon as the sysfs file for this dump is created/activated there is
>> +	 * a chance the opal_errd daemon (or any userspace) might read and
>> +	 * acknowledge the dump before kobject_uevent() is called. If that
>> +	 * happens then there is a potential race between
>> +	 * dump_ack_store->kobject_put() and kobject_uevent() which leads to a
>> +	 * use-after-free of a kernfs object resulting in a kernel crash.
>> +	 *
>> +	 * To avoid that, we need to take a reference on behalf of the bin file,
>> +	 * so that our reference remains valid while we call kobject_uevent().
>> +	 * We then drop our reference before exiting the function, leaving the
>> +	 * bin file to drop the last reference (if it hasn't already).
>> +	 */
>> +
>> +	/* Take a reference for the bin file */
>> +	kobject_get(&dump->kobj);
>>   	rc = sysfs_create_bin_file(&dump->kobj, &dump->dump_attr);
>>   	if (rc) {
>>   		kobject_put(&dump->kobj);
>> -		return NULL;
>> +		/* Drop reference count taken for bin file */
>> +		kobject_put(&dump->kobj);
>> +		return;
>>   	}
>>   
>>   	pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
>>   		__func__, dump->id, dump->size);
>>   
>>   	kobject_uevent(&dump->kobj, KOBJ_ADD);
>> -
>> -	return dump;
>> +	/* Drop reference count taken for bin file */
>> +	kobject_put(&dump->kobj);
>>   }
> 
> I think this would be better if it was reworked along the lines of:
> 
> aea948bb80b4 ("powerpc/powernv/elog: Fix race while processing OPAL error log event.")

Sure. Will fix it in v2.

-Vasant