diff mbox series

[1/3] sysfs: Fix crash on empty group attributes array

Message ID 170863445442.1479840.1818801787239831650.stgit@dwillia2-xfh.jf.intel.com
State New
Headers
Series sysfs: Group visibility fixups |

Commit Message

Dan Williams Feb. 22, 2024, 8:40 p.m. UTC 
  It turns out that arch/x86/events/intel/core.c makes use of "empty"
attributes.

	static struct attribute *empty_attrs;

	__init int intel_pmu_init(void)
	{
	        struct attribute **extra_skl_attr = &empty_attrs;
	        struct attribute **extra_attr = &empty_attrs;
	        struct attribute **td_attr    = &empty_attrs;
	        struct attribute **mem_attr   = &empty_attrs;
	        struct attribute **tsx_attr   = &empty_attrs;
		...

That breaks the assumption __first_visible() that expects that if
grp->attrs is set then grp->attrs[0] must also be set and results in
backtraces like:

    BUG: kernel NULL pointer dereference, address: 00rnel mode
    #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
    CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
     ? exc_page_fault+0x68/0x190
     internal_create_groups+0x42/0xa0
     pmu_dev_alloc+0xc0/0xe0
     perf_event_sysfs_init+0x580000000000 ]---
    RIP: 0010:exra_is_visible+0x14/0

Check for non-empty attributes array before calling is_visible().

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
Cc: Marc Herbert <marc.herbert@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/sysfs/group.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki Feb. 22, 2024, 9:14 p.m. UTC | #1 
On Thu, Feb 22, 2024 at 9:40 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> It turns out that arch/x86/events/intel/core.c makes use of "empty"
> attributes.
>
>         static struct attribute *empty_attrs;
>
>         __init int intel_pmu_init(void)
>         {
>                 struct attribute **extra_skl_attr = &empty_attrs;
>                 struct attribute **extra_attr = &empty_attrs;
>                 struct attribute **td_attr    = &empty_attrs;
>                 struct attribute **mem_attr   = &empty_attrs;
>                 struct attribute **tsx_attr   = &empty_attrs;
>                 ...
>
> That breaks the assumption __first_visible() that expects that if
> grp->attrs is set then grp->attrs[0] must also be set and results in
> backtraces like:
>
>     BUG: kernel NULL pointer dereference, address: 00rnel mode
>     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
>     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
>      ? exc_page_fault+0x68/0x190
>      internal_create_groups+0x42/0xa0
>      pmu_dev_alloc+0xc0/0xe0
>      perf_event_sysfs_init+0x580000000000 ]---
>     RIP: 0010:exra_is_visible+0x14/0
>
> Check for non-empty attributes array before calling is_visible().
>
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")

This is not in the mainline, so linux-next I suppose?

> Cc: Marc Herbert <marc.herbert@intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/sysfs/group.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index ccb275cdabcb..8c63ba3cfc47 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
>
>  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
>  {
> -       if (grp->attrs && grp->is_visible)
> +       if (grp->attrs && grp->attrs[0] && grp->is_visible)
>                 return grp->is_visible(kobj, grp->attrs[0], 0);
>
> -       if (grp->bin_attrs && grp->is_bin_visible)
> +       if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
>                 return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
>
>         return 0;
>
Dan Williams Feb. 22, 2024, 10:03 p.m. UTC | #2 
Rafael J. Wysocki wrote:
> On Thu, Feb 22, 2024 at 9:40 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > It turns out that arch/x86/events/intel/core.c makes use of "empty"
> > attributes.
> >
> >         static struct attribute *empty_attrs;
> >
> >         __init int intel_pmu_init(void)
> >         {
> >                 struct attribute **extra_skl_attr = &empty_attrs;
> >                 struct attribute **extra_attr = &empty_attrs;
> >                 struct attribute **td_attr    = &empty_attrs;
> >                 struct attribute **mem_attr   = &empty_attrs;
> >                 struct attribute **tsx_attr   = &empty_attrs;
> >                 ...
> >
> > That breaks the assumption __first_visible() that expects that if
> > grp->attrs is set then grp->attrs[0] must also be set and results in
> > backtraces like:
> >
> >     BUG: kernel NULL pointer dereference, address: 00rnel mode
> >     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
> >     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
> >      ? exc_page_fault+0x68/0x190
> >      internal_create_groups+0x42/0xa0
> >      pmu_dev_alloc+0xc0/0xe0
> >      perf_event_sysfs_init+0x580000000000 ]---
> >     RIP: 0010:exra_is_visible+0x14/0
> >
> > Check for non-empty attributes array before calling is_visible().
> >
> > Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> > Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
> 
> This is not in the mainline, so linux-next I suppose?

..or at least it will be shortly. Greg notified that it is currently in
driver-core-next:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=70317fd24b41
Dan Williams Feb. 22, 2024, 11:15 p.m. UTC | #3 
Dan Williams wrote:
> It turns out that arch/x86/events/intel/core.c makes use of "empty"
> attributes.
> 
> 	static struct attribute *empty_attrs;
> 
> 	__init int intel_pmu_init(void)
> 	{
> 	        struct attribute **extra_skl_attr = &empty_attrs;
> 	        struct attribute **extra_attr = &empty_attrs;
> 	        struct attribute **td_attr    = &empty_attrs;
> 	        struct attribute **mem_attr   = &empty_attrs;
> 	        struct attribute **tsx_attr   = &empty_attrs;
> 		...
> 
> That breaks the assumption __first_visible() that expects that if
> grp->attrs is set then grp->attrs[0] must also be set and results in
> backtraces like:
> 
>     BUG: kernel NULL pointer dereference, address: 00rnel mode
>     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
>     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
>      ? exc_page_fault+0x68/0x190
>      internal_create_groups+0x42/0xa0
>      pmu_dev_alloc+0xc0/0xe0
>      perf_event_sysfs_init+0x580000000000 ]---
>     RIP: 0010:exra_is_visible+0x14/0
> 
> Check for non-empty attributes array before calling is_visible().
> 
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
> Cc: Marc Herbert <marc.herbert@intel.com>

Ok, this one is now

Tested-by: Marc Herbert <marc.herbert@intel.com>

..per:

https://github.com/thesofproject/linux/pull/4799#issuecomment-1960469389

Thanks for all the help Marc!
diff mbox series

Patch

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index ccb275cdabcb..8c63ba3cfc47 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -33,10 +33,10 @@  static void remove_files(struct kernfs_node *parent,
 
 static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
 {
-	if (grp->attrs && grp->is_visible)
+	if (grp->attrs && grp->attrs[0] && grp->is_visible)
 		return grp->is_visible(kobj, grp->attrs[0], 0);
 
-	if (grp->bin_attrs && grp->is_bin_visible)
+	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
 		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
 
 	return 0;