LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump
@ 2020-10-07 10:17 Vasant Hegde
  2020-10-09  5:06 ` Michael Ellerman
  0 siblings, 1 reply; 3+ messages in thread
From: Vasant Hegde @ 2020-10-07 10:17 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Vasant Hegde, Mahesh Salgaonkar

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump
  2020-10-07 10:17 [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump Vasant Hegde
@ 2020-10-09  5:06 ` Michael Ellerman
  2020-10-17 12:26   ` Vasant Hegde
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Ellerman @ 2020-10-09  5:06 UTC (permalink / raw)
  To: Vasant Hegde, linuxppc-dev; +Cc: Vasant Hegde, Mahesh Salgaonkar

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump
  2020-10-09  5:06 ` Michael Ellerman
@ 2020-10-17 12:26   ` Vasant Hegde
  0 siblings, 0 replies; 3+ messages in thread
From: Vasant Hegde @ 2020-10-17 12:26 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev; +Cc: Mahesh Salgaonkar

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 10:17 [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump Vasant Hegde
2020-10-09  5:06 ` Michael Ellerman
2020-10-17 12:26   ` Vasant Hegde

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git