Re: [PATCH] umh: fix refcount underflow in fork_usermode_blob().

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2020/03/12 23:38, Al Viro wrote:
> 	It _does_ look like that double-fput() is real, but
> I'd like a confirmation before going further - umh is convoluted
> enough for something subtle to be hidden there.  Alexei, what
> the refcounting behaviour was supposed to be?  As in "this
> function consumes the reference passed to it in this argument",
> etc.
> 

Yes, double-fput() is easily observable as POISON_FREE pattern
using debug printk() patch and sample kernel module shown below.

---------- debug printk() patch start ----------
diff --git a/kernel/umh.c b/kernel/umh.c
index 7f255b5a8845..8313736b39b9 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -507,6 +507,7 @@ int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
+	printk("writecount=%d fcount=%ld\n", atomic_read(&file_inode(file)->i_writecount), atomic_long_read(&file->f_count));
 	written = kernel_write(file, data, len, &pos);
 	if (written != len) {
 		err = written;
@@ -522,12 +523,15 @@ int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
 		goto out;
 
 	err = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
+	printk("err=%d writecount=%d fcount=%ld\n", err, atomic_read(&file_inode(file)->i_writecount), atomic_long_read(&file->f_count));
 	if (!err) {
 		mutex_lock(&umh_list_lock);
 		list_add(&info->list, &umh_list);
 		mutex_unlock(&umh_list_lock);
 	}
 out:
+	schedule_timeout_killable(HZ);
+	printk("err=%d writecount=%d fcount=%ld\n", err, atomic_read(&file_inode(file)->i_writecount), atomic_long_read(&file->f_count));
 	fput(file);
 	return err;
 }
---------- debug printk() patch end ----------

---------- test.c start ----------
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/umh.h>

static int __init test_init(void)
{
	struct umh_info *info = kzalloc(sizeof(*info), GFP_KERNEL);

	printk("fork_usermode_blob()=%d\n", fork_usermode_blob((void *) "#!/bin/true\n", 12, info));
	return -EINVAL;
}

module_init(test_init);
MODULE_LICENSE("GPL");
---------- test.c end ----------

---------- without refcount-fix patch ----------
[   30.775698] test: loading out-of-tree module taints kernel.
[   30.792584] writecount=0 fcount=1
[   30.794844] err=0 writecount=1 fcount=0
[   31.843778] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC
[   31.847903] CPU: 3 PID: 4131 Comm: insmod Tainted: G           O      5.6.0-rc5+ #978
[   31.850292] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019
[   31.853578] RIP: 0010:fork_usermode_blob+0xaa/0x190
[   31.855248] Code: 41 89 c4 78 06 41 bc f4 ff ff ff bf e8 03 00 00 e8 ab 75 61 00 48 8b 43 20 48 8b 8b 80 00 00 00 44 89 e6 48 c7 c7 98 ec db b6 <8b> 90 20 02 00 00 31 c0 e8 92 35 05 00 48 89 df e8 61 ab 1c 00 44
[   31.860535] RSP: 0018:ffffba7586287c48 EFLAGS: 00010296
[   31.862173] RAX: 6b6b6b6b6b6b6b6b RBX: ffffa124e05df0c0 RCX: 6b6b6b6b6b6b6b6b
[   31.864457] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffb6dbec98
[   31.866585] RBP: ffffba7586287c78 R08: 0000000000000000 R09: 0000000000000000
[   31.868966] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[   31.871051] R13: ffffa124d4ef1a40 R14: ffffffffc0541024 R15: ffffba7586287e68
[   31.873188] FS:  00007fc8f6c2b740(0000) GS:ffffa124e7a00000(0000) knlGS:0000000000000000
[   31.875748] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   31.877856] CR2: 000055b90eba7e38 CR3: 000000021e06b005 CR4: 00000000003606e0
[   31.879968] Call Trace:
[   31.880842]  ? 0xffffffffc054d000
[   31.881943]  test_init+0x2e/0x1000 [test]
[   31.883207]  do_one_initcall+0x6f/0x360
[   31.884431]  ? kmem_cache_alloc_trace+0x2d8/0x380
[   31.885926]  do_init_module+0x5b/0x210
[   31.887137]  load_module+0x16b6/0x1d50
[   31.888384]  ? m_show+0x1d0/0x1d0
[   31.889524]  __do_sys_finit_module+0xa9/0x100
[   31.890965]  __x64_sys_finit_module+0x15/0x20
[   31.892387]  do_syscall_64+0x4a/0x1e0
[   31.893601]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   31.895119] RIP: 0033:0x7fc8f60ffba9
[   31.896305] Code: 01 00 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 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 8b 0d 97 e2 2c 00 f7 d8 64 89 01 48
[   31.901836] RSP: 002b:00007ffe67506d88 EFLAGS: 00000206 ORIG_RAX: 0000000000000139
[   31.904191] RAX: ffffffffffffffda RBX: 0000000001a671e0 RCX: 00007fc8f60ffba9
[   31.906317] RDX: 0000000000000000 RSI: 000000000041a96e RDI: 0000000000000003
[   31.908417] RBP: 000000000041a96e R08: 0000000000000000 R09: 00007ffe67506f28
[   31.910563] R10: 0000000000000003 R11: 0000000000000206 R12: 0000000000000000
[   31.913395] R13: 0000000001a66120 R14: 0000000000000000 R15: 0000000000000000
[   31.916120] Modules linked in: test(O+) af_packet nf_conntrack nf_defrag_ipv4 sunrpc mousedev vmw_balloon intel_rapl_perf input_leds led_class psmouse pcspkr sg vmw_vmci intel_agp intel_gtt i2c_piix4 rtc_cmos evdev mac_hid ac button ip_tables x_tables xfs libcrc32c crc32c_generic sd_mod ata_generic pata_acpi vmwgfx drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops ahci cfbcopyarea mptspi libahci scsi_transport_spi fb fbdev mptscsih ttm mptbase ata_piix drm nvme drm_panel_orientation_quirks nvme_core libata t10_pi agpgart e1000 i2c_core scsi_mod serio_raw atkbd libps2 i8042 serio unix ipv6 crc_ccitt nf_defrag_ipv6
[   31.936929] ---[ end trace 8073d6b33b84006e ]---
[   31.939179] RIP: 0010:fork_usermode_blob+0xaa/0x190
[   31.941431] Code: 41 89 c4 78 06 41 bc f4 ff ff ff bf e8 03 00 00 e8 ab 75 61 00 48 8b 43 20 48 8b 8b 80 00 00 00 44 89 e6 48 c7 c7 98 ec db b6 <8b> 90 20 02 00 00 31 c0 e8 92 35 05 00 48 89 df e8 61 ab 1c 00 44
[   31.949189] RSP: 0018:ffffba7586287c48 EFLAGS: 00010296
[   31.951763] RAX: 6b6b6b6b6b6b6b6b RBX: ffffa124e05df0c0 RCX: 6b6b6b6b6b6b6b6b
[   31.954909] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffb6dbec98
[   31.957834] RBP: ffffba7586287c78 R08: 0000000000000000 R09: 0000000000000000
[   31.960818] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[   31.963821] R13: ffffa124d4ef1a40 R14: ffffffffc0541024 R15: ffffba7586287e68
[   31.966644] FS:  00007fc8f6c2b740(0000) GS:ffffa124e7a00000(0000) knlGS:0000000000000000
[   31.970102] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   31.972786] CR2: 000055b90eba7e38 CR3: 000000021e06b005 CR4: 00000000003606e0
---------- without refcount-fix patch ----------

And this refcount fix patch makes consistent for both success and fail cases.

---------- with refcount-fix patch and without tomoyo ----------
[   26.901802] test: loading out-of-tree module taints kernel.
[   26.917614] writecount=0 fcount=1
[   26.922823] err=0 writecount=0 fcount=1
[   27.984948] err=0 writecount=0 fcount=1
[   27.986274] fork_usermode_blob()=0
---------- with refcount-fix patch and without tomoyo ----------

---------- with refcount-fix patch and with tomoyo ----------
[   24.784915] test: loading out-of-tree module taints kernel.
[   24.788705] writecount=0 fcount=1
[   24.790663] err=-2 writecount=0 fcount=1
[   25.796113] err=-2 writecount=0 fcount=1
[   25.799777] fork_usermode_blob()=-2
---------- with refcount-fix patch and with tomoyo ----------

Well, this -ENOENT assumption was introduced by commit 26a2a1c9eb88d9ac ("Domain transition handler.") in 2.6.30
and remains broken since commit 51f39a1f0cea1cac ("syscalls: implement execveat() system call") in 3.19...