Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
Mahesh Salgaonkar 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
> Signed-off-by: Mahesh Salgaonkar
> Signed-off-by: Aneesh Kumar K.V
> ---
> 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(>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(>kobj, >raw_attr);
> if (rc) {
> + kobject_put(>kobj);
> + /* Drop the extra reference count */
> kobject_put(>kobj);
> return NULL;
> }
>
> kobject_uevent(>kobj, KOBJ_ADD);
> + /* Drop the extra reference count */
> + kobject_put(>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
[PATCH v3] powernv/elog: Fix the race while processing OPAL error log event.
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().
The function create_elog_obj() returns the elog object which if used by
caller function will end up in use-after-free problem again. However, the
return value of create_elog_obj() function isn't being used today and there
is need as well. Hence change it to return void to make this fix complete.
Fixes: 774fea1a38c6 ("powerpc/powernv: Read OPAL error log and export it
through sysfs")
Cc: # v3.15+
Reported-by: Oliver O'Halloran
Signed-off-by: Mahesh Salgaonkar
Signed-off-by: Aneesh Kumar K.V
Reviewed-by: Oliver O'Halloran
Reviewed-by: Vasant Hegde
---
Change in v3:
- Change create_elog_obj function signature to return void.
Change in v2:
- Instead of mutex and use extra reference count on kobject to avoid the
race.
---
arch/powerpc/platforms/powernv/opal-elog.c | 25 -
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-elog.c
b/arch/powerpc/platforms/powernv/opal-elog.c
index 62ef7ad995da..e61cbf08e17e 100644
--- a/arch/powerpc/platforms/powernv/opal-elog.c
+++ b/arch/powerpc/platforms/powernv/opal-elog.c
@@ -179,14 +179,14 @@ static ssize_t raw_attr_read(struct file *filep, struct
kobject *kobj,
return count;
}
-static struct elog_obj *create_elog_obj(uint64_t id, size_t size, uint64_t
type)
+static void create_elog_obj(uint64_t id, size_t size, uint64_t type)
{
struct elog_obj *elog;
int rc;
elog = kzalloc(sizeof(*elog), GFP_KERNEL);
if (!elog)
- return NULL;
+ return;
elog->kobj.kset = elog_kset;
@@ -219,18 +219,33 @@ static struct elog_obj *create_elog_obj(uint64_t id,
size_t size, uint64_t type)
rc = kobject_add(>kobj, NULL, "0x%llx", id);
if (rc) {
kobject_put(>kobj);
- return NULL;
+ return;
}
+ /*
+* 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(>kobj); /* extra reference count */
rc = sysfs_create_bin_file(>kobj, >raw_attr);
if (rc) {
kobject_put(>kobj);
- return NULL;
+ /* Drop the extra reference count */
+ kobject_put(>kobj);
+ return;
}
kobject_uevent(>kobj, KOBJ_ADD);
+ /* Drop the extra reference count */
+ kobject_put(>kobj);
- return elog;
+ return;
}
static irqreturn_t elog_event(int irq, void *data)
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
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 >> Signed-off-by: Mahesh Salgaonkar >> Signed-off-by: Aneesh Kumar K.V > > cc stable? > Will add it in v3. Thanks, -Mahesh.
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
On 10/6/20 5:55 AM, Oliver O'Halloran wrote:
> On Mon, Oct 5, 2020 at 3:12 PM 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
>> Signed-off-by: Mahesh Salgaonkar
>> Signed-off-by: Aneesh Kumar K.V
>> ---
>> 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(>kobj); /* extra reference count */
>> rc = sysfs_create_bin_file(>kobj, >raw_attr);
>> if (rc) {
>> + kobject_put(>kobj);
>> + /* Drop the extra reference count */
>> kobject_put(>kobj);
>> return NULL;
>> }
>>
>> kobject_uevent(>kobj, KOBJ_ADD);
>> + /* Drop the extra reference count */
>> + kobject_put(>kobj);
>
> Makes sense,
>
> Reviewed-by: Oliver O'Halloran
>
>>
>> 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: [RFC PATCH next-20200930] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Tue, 2020-10-06 at 00:34 +, Joel Stanley wrote: > arch/powerpc/boot is the powerpc wrapper, and it's not built with the > same includes or flags as the rest of the kernel. It doesn't include > any of the headers in the top level include/ directory for hysterical > raisins. > > The straightforward fix would be to exclude this directory from your script. I agree and that's why I submitted another script that does just that. https://lore.kernel.org/lkml/75393e5ddc272dc7403de74d645e6c6e0f4e70eb.ca...@perches.com/
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
On 10/6/20 5:55 AM, Oliver O'Halloran wrote:
On Mon, Oct 5, 2020 at 3:12 PM 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
Signed-off-by: Mahesh Salgaonkar
Signed-off-by: Aneesh Kumar K.V
---
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(>kobj); /* extra reference count */
rc = sysfs_create_bin_file(>kobj, >raw_attr);
if (rc) {
+ kobject_put(>kobj);
+ /* Drop the extra reference count */
kobject_put(>kobj);
return NULL;
}
kobject_uevent(>kobj, KOBJ_ADD);
+ /* Drop the extra reference count */
+ kobject_put(>kobj);
Makes sense,
Reviewed-by: Oliver O'Halloran
Reviewed-by: Vasant Hegde
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: [RFC PATCH next-20200930] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Thu, Oct 1, 2020 at 1:19 PM Joe Perches wrote: > > On Thu, 2020-10-01 at 14:39 -0500, Segher Boessenkool wrch/ote: > > Hi! > > > > On Thu, Oct 01, 2020 at 12:15:39PM +0200, Miguel Ojeda wrote: > > > > So it looks like the best option is to exclude these > > > > 2 files from conversion. > > > > > > Agreed. Nevertheless, is there any reason arch/powerpc/* should not be > > > compiling cleanly with compiler.h? (CC'ing the rest of the PowerPC > > > reviewers and ML). > > > > You need to #include compiler_types.h to get this #define? > > Actually no, you need to add > > #include > > to both files and then it builds properly. > > Ideally though nothing should include this file directly. That's because Kbuild injects it via command line flag `-include`. (Well, it injects compiler_types.h which includes this). If part of the tree reset KBUILD_CFLAGS, that `-include` gets dropped. I don't think there's anything wrong with manually including it and adding `-I ` (capital i) if needed. > > > (The twice-defined thing is a warning, not an error. It should be fixed > > of course, but it is less important; although it may be pointing to a > > deeper problem.) > > > > > > Segher > -- Thanks, ~Nick Desaulniers
Re: [RFC PATCH next-20200930] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Thu, 1 Oct 2020 at 20:19, Joe Perches wrote: > > On Thu, 2020-10-01 at 14:39 -0500, Segher Boessenkool wrch/ote: > > Hi! > > > > On Thu, Oct 01, 2020 at 12:15:39PM +0200, Miguel Ojeda wrote: > > > > So it looks like the best option is to exclude these > > > > 2 files from conversion. > > > > > > Agreed. Nevertheless, is there any reason arch/powerpc/* should not be > > > compiling cleanly with compiler.h? (CC'ing the rest of the PowerPC > > > reviewers and ML). > > > > You need to #include compiler_types.h to get this #define? > > Actually no, you need to add > > #include > > to both files and then it builds properly. > > Ideally though nothing should include this file directly. arch/powerpc/boot is the powerpc wrapper, and it's not built with the same includes or flags as the rest of the kernel. It doesn't include any of the headers in the top level include/ directory for hysterical raisins. The straightforward fix would be to exclude this directory from your script. Cheers, Joel
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
On Mon, Oct 5, 2020 at 3:12 PM 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
> Signed-off-by: Mahesh Salgaonkar
> Signed-off-by: Aneesh Kumar K.V
> ---
> 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(>kobj); /* extra reference count */
> rc = sysfs_create_bin_file(>kobj, >raw_attr);
> if (rc) {
> + kobject_put(>kobj);
> + /* Drop the extra reference count */
> kobject_put(>kobj);
> return NULL;
> }
>
> kobject_uevent(>kobj, KOBJ_ADD);
> + /* Drop the extra reference count */
> + kobject_put(>kobj);
Makes sense,
Reviewed-by: Oliver O'Halloran
>
> 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.
On Mon, Oct 5, 2020 at 11:07 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 > > Signed-off-by: Mahesh Salgaonkar > > Signed-off-by: Aneesh Kumar K.V > > cc stable? +1
Re: [RFC PATCH next-20200930] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Mon, 2020-10-05 at 11:36 -0700, Nick Desaulniers wrote:
> I don't think there's anything wrong with manually including it and adding `-I
> ` (capital i) if needed.
All of this is secondary to the actual change to use
quoted __section("foo") rather than __section(foo)
I'd rather get that done first and then figure out if
additional changes could be done later.
Re: [PATCH v2] powernv/elog: Fix the race while processing OPAL error log event.
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 Signed-off-by: Mahesh Salgaonkar Signed-off-by: Aneesh Kumar K.V cc stable? -- Ananth
