On Mon, Oct 5, 2020 at 3:12 PM Mahesh Salgaonkar <mah...@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 <ooh...@gmail.com>
> Signed-off-by: Mahesh Salgaonkar <mah...@linux.ibm.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@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 <ooh...@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.
