Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio: dummy_evgen: check iio_evgen in iio_dummy_evgen_free()
@ 2019-05-09  2:04 Kefeng Wang
  2019-05-11  8:58 ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Kefeng Wang @ 2019-05-09  2:04 UTC (permalink / raw)
  To: Jonathan Cameron, Jonathan Cameron, linux-iio, linux-kernel,
	Rodrigo Siqueira
  Cc: Kefeng Wang, Hulk Robot

if iio_dummy_evgen_create() fails, iio_evgen should be NULL, when call
iio_evgen_release() to cleanup, it throws some warning and could cause
double free.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/iio/dummy/iio_dummy_evgen.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c
index c6033e341963..2327b5f52086 100644
--- a/drivers/iio/dummy/iio_dummy_evgen.c
+++ b/drivers/iio/dummy/iio_dummy_evgen.c
@@ -58,6 +58,7 @@ static int iio_dummy_evgen_create(void)
 	ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO);
 	if (ret < 0) {
 		kfree(iio_evgen);
+		iio_evgen = NULL;
 		return ret;
 	}
 
@@ -118,6 +119,9 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
 
 static void iio_dummy_evgen_free(void)
 {
+	if (!iio_evgen)
+		return;
+
 	irq_sim_fini(&iio_evgen->irq_sim);
 	kfree(iio_evgen);
 }
-- 
2.20.1


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

* Re: [PATCH] iio: dummy_evgen: check iio_evgen in iio_dummy_evgen_free()
  2019-05-09  2:04 [PATCH] iio: dummy_evgen: check iio_evgen in iio_dummy_evgen_free() Kefeng Wang
@ 2019-05-11  8:58 ` Jonathan Cameron
       [not found]   ` <c691dfa4-4490-d643-e184-ea487bcbea94@huawei.com>
  0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2019-05-11  8:58 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: Jonathan Cameron, linux-iio, linux-kernel, Rodrigo Siqueira, Hulk Robot

On Thu, 9 May 2019 10:04:47 +0800
Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> if iio_dummy_evgen_create() fails, iio_evgen should be NULL, when call
> iio_evgen_release() to cleanup, it throws some warning and could cause
> double free.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Hi Kefeng,

I'm not seeing a path to be able to trigger this.
iio_dummy_evgen_create is called only in the module_init.
If it fails, then the init fails before the device
initialization call is made.

How would we then be running the device release call
in order to end up freeing this again?

So I think this is a false positive but perhaps there is
a path that I am missing.

Jonathan

> ---
>  drivers/iio/dummy/iio_dummy_evgen.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c
> index c6033e341963..2327b5f52086 100644
> --- a/drivers/iio/dummy/iio_dummy_evgen.c
> +++ b/drivers/iio/dummy/iio_dummy_evgen.c
> @@ -58,6 +58,7 @@ static int iio_dummy_evgen_create(void)
>  	ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO);
>  	if (ret < 0) {
>  		kfree(iio_evgen);
> +		iio_evgen = NULL;
>  		return ret;
>  	}
>  
> @@ -118,6 +119,9 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
>  
>  static void iio_dummy_evgen_free(void)
>  {
> +	if (!iio_evgen)
> +		return;
> +
>  	irq_sim_fini(&iio_evgen->irq_sim);
>  	kfree(iio_evgen);
>  }


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

* Re: [PATCH] iio: dummy_evgen: check iio_evgen in iio_dummy_evgen_free()
       [not found]   ` <c691dfa4-4490-d643-e184-ea487bcbea94@huawei.com>
@ 2019-07-28  9:51     ` Jonathan Cameron
  0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2019-07-28  9:51 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: Jonathan Cameron, linux-iio, linux-kernel, Rodrigo Siqueira, Hulk Robot

On Sat, 11 May 2019 20:14:24 +0800
Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> On 2019/5/11 16:58, Jonathan Cameron wrote:
> > On Thu, 9 May 2019 10:04:47 +0800
> > Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >  
> >> if iio_dummy_evgen_create() fails, iio_evgen should be NULL, when call
> >> iio_evgen_release() to cleanup, it throws some warning and could cause
> >> double free.
> >>
> >> Reported-by: Hulk Robot <hulkci@huawei.com>
> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>  
> > Hi Kefeng,
> >
> > I'm not seeing a path to be able to trigger this.
> > iio_dummy_evgen_create is called only in the module_init.
> > If it fails, then the init fails before the device
> > initialization call is made.
> >
> > How would we then be running the device release call
> > in order to end up freeing this again?
> >
> > So I think this is a false positive but perhaps there is
> > a path that I am missing.  
> 
> Hi Jonathan,
> 
> Here is one of out inner test robot log, could you check it?

Sorry it's taking me an extremely long time to get to you on this!
This one always comes bottom of my heap as it's a demo driver
only so can't break anyone's real systems.

There is clearly an issue here as you identified. I actually think
it's worse than you have highlighted because that global pointer
is accessed from other modules and so could cause a more direct
problem.  For that we probably need to ensure we only set the
global pointer after we are sure we have succeeded. (so use a local
variable until that point).

I'm still a little confused on the path that leads to the dump
below, but given there is clearly a bug anyway so the
exact trigger doesn't really matter :)

My confusion is that a failure in a module_init should result
in the module being removed in do_init_module drivers/base/module.c.
So the question is are we triggering in that path or in an explicit
attempt to remove the module later.

My suspicion is that we are fixing this partly at the wrong level.

The device_unregister is being called on a device that
was never registered (though I'm not certain why)

If it's correct that exit should be called after a failed init
via some path, then a level of indirection or a flag to say if the
device is registered yet should solve the particular case you hit.

Jonathan

> 
> 
> [ 2606.472327] FAULT_INJECTION: forcing a failure.
> [ 2606.472327] name failslab, interval 1, probability 0, space 0, times 0
> [ 2606.476620] CPU: 0 PID: 7076 Comm: syz-executor.0 Tainted: G         C        5.1.0+ #28
> [ 2606.477571] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> [ 2606.477571] Call Trace:
> [ 2606.477571]  dump_stack+0xa9/0x10e
> [ 2606.477571]  should_fail+0x3ca/0x3f0
> [ 2606.477571]  ? fault_create_debugfs_attr+0x1c0/0x1c0
> [ 2606.477571]  ? lock_downgrade+0x2d0/0x2d0
> [ 2606.477571]  ? pageset_set_high_and_batch+0xd0/0xd0
> [ 2606.477571]  ? __kernfs_new_node+0xbb/0x3a0
> [ 2606.477571]  __should_failslab+0x88/0xa0
> [ 2606.477571]  should_failslab+0x5/0x10
> [ 2606.477571]  kmem_cache_alloc+0x3e/0x240
> [ 2606.493436]  __kernfs_new_node+0xbb/0x3a0
> [ 2606.493436]  ? kernfs_dop_revalidate+0x1b0/0x1b0
> [ 2606.493436]  ? __mutex_unlock_slowpath+0xb4/0x3f0
> [ 2606.493436]  ? wait_for_completion+0x240/0x240
> [ 2606.493436]  ? kernfs_next_descendant_post+0xbe/0x190
> [ 2606.493436]  kernfs_new_node+0x6c/0xc0
> [ 2606.493436]  __kernfs_create_file+0x56/0x1b0
> [ 2606.493436]  sysfs_add_file_mode_ns+0x160/0x300
> [ 2606.493436]  internal_create_group+0x255/0x6e0
> [ 2606.493436]  ? remove_files.isra.1+0xb0/0xb0
> [ 2606.493436]  ? sysfs_create_dir_ns+0x100/0x1e0
> [ 2606.493436]  ? sysfs_create_dir_ns+0x12b/0x1e0
> [ 2606.493436]  ? sysfs_create_mount_point+0x90/0x90
> [ 2606.493436]  sysfs_create_groups+0x6d/0xe0
> [ 2606.493436]  kobject_add_internal+0x355/0x530
> [ 2606.493436]  ? kfree_const+0x33/0x40
> [ 2606.493436]  kobject_add+0x101/0x1a0
> [ 2606.493436]  ? kobject_add_internal+0x530/0x530
> [ 2606.493436]  ? lockdep_init_map+0x98/0x2c0
> [ 2606.493436]  ? lockdep_init_map+0x98/0x2c0
> [ 2606.493436]  ? find_next_bit+0x80/0xb0
> [ 2606.493436]  ? kobject_init+0xa3/0x100
> [ 2606.493436]  irq_sysfs_add+0x39/0x60
> [ 2606.493436]  __irq_alloc_descs+0x242/0x2d0
> [ 2606.493436]  ? 0xffffffffc1920000
> [ 2606.493436]  irq_sim_init+0x6a/0x240
> [ 2606.493436]  ? 0xffffffffc1920000
> [ 2606.493436]  iio_dummy_evgen_init+0x57/0x1000 [iio_dummy_evgen]
> [ 2606.493436]  do_one_initcall+0xb9/0x3b5
> [ 2606.493436]  ? perf_trace_initcall_level+0x270/0x270
> [ 2606.493436]  ? kasan_unpoison_shadow+0x30/0x40
> [ 2606.493436]  ? kasan_unpoison_shadow+0x30/0x40
> [ 2606.493436]  do_init_module+0xe0/0x330
> [ 2606.493436]  load_module+0x38eb/0x4270
> [ 2606.493436]  ? module_frob_arch_sections+0x20/0x20
> [ 2606.493436]  ? kernel_read_file+0x188/0x3f0
> [ 2606.493436]  ? find_held_lock+0x6d/0xd0
> [ 2606.493436]  ? fput_many+0x1a/0xe0
> [ 2606.493436]  ? __do_sys_finit_module+0x162/0x190
> [ 2606.493436]  __do_sys_finit_module+0x162/0x190
> [ 2606.493436]  ? __ia32_sys_init_module+0x40/0x40
> [ 2606.493436]  ? __mutex_unlock_slowpath+0xb4/0x3f0
> [ 2606.493436]  ? wait_for_completion+0x240/0x240
> [ 2606.493436]  ? vfs_write+0x160/0x2a0
> [ 2606.493436]  ? lockdep_hardirqs_off+0xb5/0x100
> [ 2606.493436]  ? mark_held_locks+0x1a/0x90
> [ 2606.493436]  ? do_syscall_64+0x14/0x2a0
> [ 2606.493436]  do_syscall_64+0x72/0x2a0
> [ 2606.493436]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 2606.493436] RIP: 0033:0x463de9
> [ 2606.493436] Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> [ 2606.493436] RSP: 002b:00007fa67f828c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [ 2606.493436] RAX: ffffffffffffffda RBX: 000000000073bf00 RCX: 0000000000463de9
> [ 2606.493436] RDX: 0000000000000000 RSI: 0000000020000100 RDI: 0000000000000003
> [ 2606.493436] RBP: 00007fa67f828c70 R08: 0000000000000000 R09: 0000000000000000
> [ 2606.493436] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> [ 2606.493436] R13: 00000000006f8748 R14: 00000000004bdeea R15: 00007fa67f8296bc
> [ 2606.569187] kobject_add_internal failed for 33 (error: -12 parent: irq)
> [ 2606.571027] Failed to add kobject for irq 33
> [ 2606.587831] ------------[ cut here ]------------
> [ 2606.589151] kernfs: can not remove 'per_cpu_count', no directory
> [ 2606.590937] WARNING: CPU: 0 PID: 7076 at fs/kernfs/dir.c:1505 kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.591782] Kernel panic - not syncing: panic_on_warn set ...
> [ 2606.591782] CPU: 0 PID: 7076 Comm: syz-executor.0 Tainted: G         C        5.1.0+ #28
> [ 2606.591782] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> [ 2606.599643] Call Trace:
> [ 2606.600427]  dump_stack+0xa9/0x10e
> [ 2606.600427]  panic+0x1bc/0x434
> [ 2606.602438]  ? refcount_error_report+0x10c/0x10c
> [ 2606.602438]  ? kmsg_dump_rewind_nolock+0x59/0x59
> [ 2606.602438]  ? __probe_kernel_read+0xc6/0xe0
> [ 2606.602438]  ? kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438]  __warn+0x18e/0x190
> [ 2606.602438]  ? kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438]  report_bug+0x12c/0x1b0
> [ 2606.602438]  ? kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438]  fixup_bug.part.11+0x23/0x50
> [ 2606.602438]  do_error_trap+0xc8/0x110
> [ 2606.602438]  do_invalid_op+0x31/0x40
> [ 2606.602438]  ? kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438]  invalid_op+0x14/0x20
> [ 2606.602438] RIP: 0010:kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438] Code: c7 c0 5a 9d 8e e8 43 ea a5 00 e8 2e 9a cb ff 89 d8 5b 5d 41 5c c3 e8 22 9a cb ff 48 89 ee 48 c7 c7 c0 7b 31 8e e8 53 ec b2 ff <0f> 0b bb fe ff ff ff eb b5 66 2e 0f 1f 84 00 00 00 00 00 41 57 41
> [ 2606.602438] RSP: 0018:ffff8881be38fcc8 EFLAGS: 00010282
> [ 2606.602438] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff81179529
> [ 2606.602438] RDX: 000000000000adf2 RSI: ffffc90001405000 RDI: ffff8881f742684c
> [ 2606.602438] RBP: ffffffff8e270d00 R08: ffffed103ee85e61 R09: ffffed103ee85e61
> [ 2606.602438] R10: 0000000000000000 R11: ffffed103ee85e60 R12: 0000000000000000
> [ 2606.602438] R13: ffffffff8e270b40 R14: 0000000000000000 R15: 0000000000000000
> [ 2606.602438]  ? kernfs_remove_by_name_ns+0x9d/0xb0
> [ 2606.602438]  remove_files.isra.1+0x3f/0xb0
> [ 2606.602438]  sysfs_remove_group+0x68/0xf0
> [ 2606.602438]  sysfs_remove_groups+0x41/0x70
> [ 2606.602438]  kobject_del+0x53/0xb0
> [ 2606.602438]  free_desc+0x33/0x60
> [ 2606.602438]  irq_free_descs+0x5d/0x90
> [ 2606.602438]  irq_sim_fini+0x43/0x60
> [ 2606.602438]  ? iio_dummy_evgen_release_irq+0x60/0x60 [iio_dummy_evgen]
> [ 2606.602438]  iio_evgen_release+0x18/0x30 [iio_dummy_evgen]
> [ 2606.602438]  device_release+0x46/0x100
> [ 2606.602438]  kobject_put+0x1a8/0x220
> [ 2606.602438]  device_unregister+0x23/0x30
> [ 2606.602438]  __x64_sys_delete_module+0x244/0x330
> [ 2606.602438]  ? __ia32_sys_delete_module+0x330/0x330
> [ 2606.602438]  ? __x64_sys_clock_gettime+0xe3/0x160
> [ 2606.602438]  ? trace_hardirqs_on_thunk+0x1a/0x1c
> [ 2606.602438]  ? trace_hardirqs_off_caller+0x3e/0x130
> [ 2606.602438]  ? lockdep_hardirqs_off+0xb5/0x100
> [ 2606.602438]  ? mark_held_locks+0x1a/0x90
> [ 2606.602438]  ? do_syscall_64+0x14/0x2a0
> [ 2606.602438]  do_syscall_64+0x72/0x2a0
> [ 2606.602438]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 2606.602438] RIP: 0033:0x463de9
> [ 2606.602438] Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> [ 2606.602438] RSP: 002b:00007fa67f828c58 EFLAGS: 00000246 ORIG_RAX: 00000000000000b0
> [ 2606.602438] RAX: ffffffffffffffda RBX: 000000000073bf00 RCX: 0000000000463de9
> [ 2606.602438] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000140
> [ 2606.602438] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
> [ 2606.602438] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> [ 2606.602438] R13: 00000000006f82e0 R14: 00000000004bdc99 R15: 00007fa67f8296bc
> [ 2606.602438] Dumping ftrace buffer:
> [ 2606.602438]    (ftrace buffer empty)
> [ 2606.602438] Kernel Offset: 0xbe00000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
> 
> 
> 
> 
> >
> > Jonathan  


^ 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 --
2019-05-09  2:04 [PATCH] iio: dummy_evgen: check iio_evgen in iio_dummy_evgen_free() Kefeng Wang
2019-05-11  8:58 ` Jonathan Cameron
     [not found]   ` <c691dfa4-4490-d643-e184-ea487bcbea94@huawei.com>
2019-07-28  9:51     ` Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/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 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


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