[PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Mahesh Salgaonkar]

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

However there are chances where user daemon may already be scanning elog
entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
which can lead to use-after-free issue of a kernfs object resulting into a
kernel crash. This patch fixes this race by protecting a sysfs file
creation/notification by holding an additional reference count on kobject
until we safely send kobject_uevent().

Reported-by: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
Change in v2:
- Instead of mutex and use extra reference count on kobject to avoid the
  race.
---
 arch/powerpc/platforms/powernv/opal-elog.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal-elog.c b/arch/powerpc/platforms/powernv/opal-elog.c
index 62ef7ad995da..230f102e87c0 100644
--- a/arch/powerpc/platforms/powernv/opal-elog.c
+++ b/arch/powerpc/platforms/powernv/opal-elog.c
@@ -222,13 +222,28 @@ static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t type)
 		return NULL;
 	}
 
+	/*
+	 * As soon as sysfs file for this elog is created/activated there is
+	 * chance opal_errd daemon might read and acknowledge this elog before
+	 * kobject_uevent() is called. If that happens then we have a potential
+	 * race between elog_ack_store->kobject_put() and kobject_uevent which
+	 * leads to use-after-free issue of a kernfs object resulting into
+	 * kernel crash. To avoid this race take an additional reference count
+	 * on kobject until we safely send kobject_uevent().
+	 */
+
+	kobject_get(&elog->kobj);  /* extra reference count */
 	rc = sysfs_create_bin_file(&elog->kobj, &elog->raw_attr);
 	if (rc) {
+		kobject_put(&elog->kobj);
+		/* Drop the extra reference count  */
 		kobject_put(&elog->kobj);
 		return NULL;
 	}
 
 	kobject_uevent(&elog->kobj, KOBJ_ADD);
+	/* Drop the extra reference count  */
+	kobject_put(&elog->kobj);
 
 	return elog;
 }

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Ananth N Mavinakayanahalli]

On 10/5/20 9:42 AM, Mahesh Salgaonkar wrote:
> Every error log reported by OPAL is exported to userspace through a sysfs
> interface and notified using kobject_uevent(). The userspace daemon
> (opal_errd) then reads the error log and acknowledges it error log is saved
> safely to disk. Once acknowledged the kernel removes the respective sysfs
> file entry causing respective resources getting released including kobject.
> 
> However there are chances where user daemon may already be scanning elog
> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
> which can lead to use-after-free issue of a kernfs object resulting into a
> kernel crash. This patch fixes this race by protecting a sysfs file
> creation/notification by holding an additional reference count on kobject
> until we safely send kobject_uevent().
> 
> Reported-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>

cc stable?

-- 
Ananth

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Oliver O'Halloran]

On Mon, Oct 5, 2020 at 11:07 PM Ananth N Mavinakayanahalli
<ananth@linux.ibm.com> wrote:
>
> On 10/5/20 9:42 AM, Mahesh Salgaonkar wrote:
> > Every error log reported by OPAL is exported to userspace through a sysfs
> > interface and notified using kobject_uevent(). The userspace daemon
> > (opal_errd) then reads the error log and acknowledges it error log is saved
> > safely to disk. Once acknowledged the kernel removes the respective sysfs
> > file entry causing respective resources getting released including kobject.
> >
> > However there are chances where user daemon may already be scanning elog
> > entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
> > which can lead to use-after-free issue of a kernfs object resulting into a
> > kernel crash. This patch fixes this race by protecting a sysfs file
> > creation/notification by holding an additional reference count on kobject
> > until we safely send kobject_uevent().
> >
> > Reported-by: Oliver O'Halloran <oohall@gmail.com>
> > Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> cc stable?

+1

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Oliver O'Halloran]

On Mon, Oct 5, 2020 at 3:12 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>
> Every error log reported by OPAL is exported to userspace through a sysfs
> interface and notified using kobject_uevent(). The userspace daemon
> (opal_errd) then reads the error log and acknowledges it error log is saved
> safely to disk. Once acknowledged the kernel removes the respective sysfs
> file entry causing respective resources getting released including kobject.
>
> However there are chances where user daemon may already be scanning elog
> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
> which can lead to use-after-free issue of a kernfs object resulting into a
> kernel crash. This patch fixes this race by protecting a sysfs file
> creation/notification by holding an additional reference count on kobject
> until we safely send kobject_uevent().
>
> Reported-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Change in v2:
> - Instead of mutex and use extra reference count on kobject to avoid the
>   race.
> ---
>  arch/powerpc/platforms/powernv/opal-elog.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/opal-elog.c b/arch/powerpc/platforms/powernv/opal-elog.c
> index 62ef7ad995da..230f102e87c0 100644
> --- a/arch/powerpc/platforms/powernv/opal-elog.c
> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
> @@ -222,13 +222,28 @@ static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t type)
>                 return NULL;
>         }
>
> +       /*
> +        * As soon as sysfs file for this elog is created/activated there is
> +        * chance opal_errd daemon might read and acknowledge this elog before
> +        * kobject_uevent() is called. If that happens then we have a potential
> +        * race between elog_ack_store->kobject_put() and kobject_uevent which
> +        * leads to use-after-free issue of a kernfs object resulting into
> +        * kernel crash. To avoid this race take an additional reference count
> +        * on kobject until we safely send kobject_uevent().
> +        */
> +
> +       kobject_get(&elog->kobj);  /* extra reference count */
>         rc = sysfs_create_bin_file(&elog->kobj, &elog->raw_attr);
>         if (rc) {
> +               kobject_put(&elog->kobj);
> +               /* Drop the extra reference count  */
>                 kobject_put(&elog->kobj);
>                 return NULL;
>         }
>
>         kobject_uevent(&elog->kobj, KOBJ_ADD);
> +       /* Drop the extra reference count  */
> +       kobject_put(&elog->kobj);

Makes sense,

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

>
>         return elog;

Does the returned value actually get used anywhere? We'd have a
similar use-after-free problem if it does. This should probably return
an error code rather than the object itself.

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Vasant Hegde]

On 10/6/20 5:55 AM, Oliver O'Halloran wrote:
> On Mon, Oct 5, 2020 at 3:12 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>>
>> Every error log reported by OPAL is exported to userspace through a sysfs
>> interface and notified using kobject_uevent(). The userspace daemon
>> (opal_errd) then reads the error log and acknowledges it error log is saved
>> safely to disk. Once acknowledged the kernel removes the respective sysfs
>> file entry causing respective resources getting released including kobject.
>>
>> However there are chances where user daemon may already be scanning elog
>> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
>> which can lead to use-after-free issue of a kernfs object resulting into a
>> kernel crash. This patch fixes this race by protecting a sysfs file
>> creation/notification by holding an additional reference count on kobject
>> until we safely send kobject_uevent().
>>
>> Reported-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> Change in v2:
>> - Instead of mutex and use extra reference count on kobject to avoid the
>>    race.
>> ---
>>   arch/powerpc/platforms/powernv/opal-elog.c |   15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-elog.c b/arch/powerpc/platforms/powernv/opal-elog.c
>> index 62ef7ad995da..230f102e87c0 100644
>> --- a/arch/powerpc/platforms/powernv/opal-elog.c
>> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
>> @@ -222,13 +222,28 @@ static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t type)
>>                  return NULL;
>>          }
>>
>> +       /*
>> +        * As soon as sysfs file for this elog is created/activated there is
>> +        * chance opal_errd daemon might read and acknowledge this elog before
>> +        * kobject_uevent() is called. If that happens then we have a potential
>> +        * race between elog_ack_store->kobject_put() and kobject_uevent which
>> +        * leads to use-after-free issue of a kernfs object resulting into
>> +        * kernel crash. To avoid this race take an additional reference count
>> +        * on kobject until we safely send kobject_uevent().
>> +        */
>> +
>> +       kobject_get(&elog->kobj);  /* extra reference count */
>>          rc = sysfs_create_bin_file(&elog->kobj, &elog->raw_attr);
>>          if (rc) {
>> +               kobject_put(&elog->kobj);
>> +               /* Drop the extra reference count  */
>>                  kobject_put(&elog->kobj);
>>                  return NULL;
>>          }
>>
>>          kobject_uevent(&elog->kobj, KOBJ_ADD);
>> +       /* Drop the extra reference count  */
>> +       kobject_put(&elog->kobj);
> 
> Makes sense,
> 
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

> 
>>
>>          return elog;
> 
> Does the returned value actually get used anywhere? We'd have a
> similar use-after-free problem if it does. This should probably return
> an error code rather than the object itself.

No one is using it today. May be we should just make it void function.

-Vasant

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Mahesh Jagannath Salgaonkar]

On 10/6/20 5:55 AM, Oliver O'Halloran wrote:
> On Mon, Oct 5, 2020 at 3:12 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>>
>> Every error log reported by OPAL is exported to userspace through a sysfs
>> interface and notified using kobject_uevent(). The userspace daemon
>> (opal_errd) then reads the error log and acknowledges it error log is saved
>> safely to disk. Once acknowledged the kernel removes the respective sysfs
>> file entry causing respective resources getting released including kobject.
>>
>> However there are chances where user daemon may already be scanning elog
>> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
>> which can lead to use-after-free issue of a kernfs object resulting into a
>> kernel crash. This patch fixes this race by protecting a sysfs file
>> creation/notification by holding an additional reference count on kobject
>> until we safely send kobject_uevent().
>>
>> Reported-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> Change in v2:
>> - Instead of mutex and use extra reference count on kobject to avoid the
>>   race.
>> ---
>>  arch/powerpc/platforms/powernv/opal-elog.c |   15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-elog.c b/arch/powerpc/platforms/powernv/opal-elog.c
>> index 62ef7ad995da..230f102e87c0 100644
>> --- a/arch/powerpc/platforms/powernv/opal-elog.c
>> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
>> @@ -222,13 +222,28 @@ static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t type)
>>                 return NULL;
>>         }
>>
>> +       /*
>> +        * As soon as sysfs file for this elog is created/activated there is
>> +        * chance opal_errd daemon might read and acknowledge this elog before
>> +        * kobject_uevent() is called. If that happens then we have a potential
>> +        * race between elog_ack_store->kobject_put() and kobject_uevent which
>> +        * leads to use-after-free issue of a kernfs object resulting into
>> +        * kernel crash. To avoid this race take an additional reference count
>> +        * on kobject until we safely send kobject_uevent().
>> +        */
>> +
>> +       kobject_get(&elog->kobj);  /* extra reference count */
>>         rc = sysfs_create_bin_file(&elog->kobj, &elog->raw_attr);
>>         if (rc) {
>> +               kobject_put(&elog->kobj);
>> +               /* Drop the extra reference count  */
>>                 kobject_put(&elog->kobj);
>>                 return NULL;
>>         }
>>
>>         kobject_uevent(&elog->kobj, KOBJ_ADD);
>> +       /* Drop the extra reference count  */
>> +       kobject_put(&elog->kobj);
> 
> Makes sense,
> 
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
> 
>>
>>         return elog;
> 
> Does the returned value actually get used anywhere? We'd have a
> similar use-after-free problem if it does. This should probably return
> an error code rather than the object itself.
> 

Nope. It  isn't being used. I can make it function as void and send v3.

Thanks,
-Mahesh.

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Mahesh Jagannath Salgaonkar]

On 10/5/20 4:17 PM, Ananth N Mavinakayanahalli wrote:
> On 10/5/20 9:42 AM, Mahesh Salgaonkar wrote:
>> Every error log reported by OPAL is exported to userspace through a sysfs
>> interface and notified using kobject_uevent(). The userspace daemon
>> (opal_errd) then reads the error log and acknowledges it error log is
>> saved
>> safely to disk. Once acknowledged the kernel removes the respective sysfs
>> file entry causing respective resources getting released including
>> kobject.
>>
>> However there are chances where user daemon may already be scanning elog
>> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
>> which can lead to use-after-free issue of a kernfs object resulting
>> into a
>> kernel crash. This patch fixes this race by protecting a sysfs file
>> creation/notification by holding an additional reference count on kobject
>> until we safely send kobject_uevent().
>>
>> Reported-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> 
> cc stable?
> 

Will add it in v3.

Thanks,
-Mahesh.

Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.

[Michael Ellerman]

Mahesh Salgaonkar <mahesh@linux.ibm.com> writes:
> Every error log reported by OPAL is exported to userspace through a sysfs
> interface and notified using kobject_uevent(). The userspace daemon
> (opal_errd) then reads the error log and acknowledges it error log is saved
> safely to disk. Once acknowledged the kernel removes the respective sysfs
> file entry causing respective resources getting released including kobject.
>
> However there are chances where user daemon may already be scanning elog
> entries while new sysfs elog entry is being created by 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 elog_ack_store->kobject_put() and kobject_uevent
> which can lead to use-after-free issue of a kernfs object resulting into a
> kernel crash. This patch fixes this race by protecting a sysfs file
> creation/notification by holding an additional reference count on kobject
> until we safely send kobject_uevent().
>
> Reported-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Change in v2:
> - Instead of mutex and use extra reference count on kobject to avoid the
>   race.
> ---
>  arch/powerpc/platforms/powernv/opal-elog.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/opal-elog.c b/arch/powerpc/platforms/powernv/opal-elog.c
> index 62ef7ad995da..230f102e87c0 100644
> --- a/arch/powerpc/platforms/powernv/opal-elog.c
> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
> @@ -222,13 +222,28 @@ static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t type)
>  		return NULL;
>  	}
>  
> +	/*
> +	 * As soon as sysfs file for this elog is created/activated there is
> +	 * chance opal_errd daemon might read and acknowledge this elog before
> +	 * kobject_uevent() is called. If that happens then we have a potential
> +	 * race between elog_ack_store->kobject_put() and kobject_uevent which
> +	 * leads to use-after-free issue of a kernfs object resulting into
> +	 * kernel crash. To avoid this race take an additional reference count
> +	 * on kobject until we safely send kobject_uevent().
> +	 */
> +
> +	kobject_get(&elog->kobj);  /* extra reference count */

It's not really an "extra" reference.

The way the code is currently written there's one reference and it's
given to (moved into) sysfs_create_bin_file(). (Because elog_ack_store()
drops that reference).

So after that call this function no longer has a valid reference to
kobj.

If this function wants to continue to use kobj (which it does) it needs
its own reference.

Or the other way to think about it is that this code owns the initial
reference, and it needs to take a reference on behalf of the bin file
before creating the bin file.

So the patch is not wrong, but I think the comments are a bit
misleading.


>  	rc = sysfs_create_bin_file(&elog->kobj, &elog->raw_attr);
>  	if (rc) {
> +		kobject_put(&elog->kobj);
> +		/* Drop the extra reference count  */
>  		kobject_put(&elog->kobj);
>  		return NULL;
>  	}
>  
>  	kobject_uevent(&elog->kobj, KOBJ_ADD);
> +	/* Drop the extra reference count  */
> +	kobject_put(&elog->kobj);
>  
>  	return elog;

And it is bogus that we return elog here, because we no longer have a
valid reference to it, so it may already be freed.

>  }

So please send a v3 with updated comments and the return dropped.

cheers