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

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

[Tetsuo Handa]

Before thinking how to fix a bug that tomoyo_realpath_nofollow() from
tomoyo_find_next_domain() likely fails with -ENOENT whenever
fork_usermode_blob() is used because 449325b52b7a6208 did not take into
account that TOMOYO security module needs to calculate symlink's pathname,
is this a correct fix for a bug that file_inode(file)->i_writecount != 0
and file->f_count < 0 ?



From 8a9891af757a89b2a52addbc88a9911c17f6a2a9 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 12 Mar 2020 22:39:26 +0900
Subject: [PATCH] umh: fix refcount underflow in fork_usermode_blob().

Since free_bprm(bprm) always calls allow_write_access(bprm->file) and
fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file()
must call deny_write_access(file) and get_file(file) if called from
do_execve_file() path.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 449325b52b7a6208 ("umh: introduce fork_usermode_blob() helper")
---
 fs/exec.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..ded3fa368dc7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename,
 	check_unsafe_exec(bprm);
 	current->in_execve = 1;
 
-	if (!file)
+	if (!file) {
 		file = do_open_execat(fd, filename, flags);
-	retval = PTR_ERR(file);
-	if (IS_ERR(file))
-		goto out_unmark;
+		retval = PTR_ERR(file);
+		if (IS_ERR(file))
+			goto out_unmark;
+	} else {
+		retval = deny_write_access(file);
+		if (retval)
+			goto out_unmark;
+		get_file(file);
+	}
 
 	sched_exec();
 
-- 
2.18.2

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

[Al Viro]

On Thu, Mar 12, 2020 at 10:43:00PM +0900, Tetsuo Handa wrote:
> Before thinking how to fix a bug that tomoyo_realpath_nofollow() from
> tomoyo_find_next_domain() likely fails with -ENOENT whenever
> fork_usermode_blob() is used because 449325b52b7a6208 did not take into
> account that TOMOYO security module needs to calculate symlink's pathname,
> is this a correct fix for a bug that file_inode(file)->i_writecount != 0
> and file->f_count < 0 ?

> -	if (!file)
> +	if (!file) {
>  		file = do_open_execat(fd, filename, flags);
> -	retval = PTR_ERR(file);
> -	if (IS_ERR(file))
> -		goto out_unmark;
> +		retval = PTR_ERR(file);
> +		if (IS_ERR(file))
> +			goto out_unmark;
> +	} else {
> +		retval = deny_write_access(file);
> +		if (retval)
> +			goto out_unmark;
> +		get_file(file);
> +	}

*UGH*

	Something's certainly fishy with the refcounting there.
First of all, bprm->file is a counting reference (observe what
free_bprm() is doing).  So as it is, on success __do_execve_file()
consumes the reference passed to it in 'file', ditto for
do_execve_file().  However, it's inconsistent - failure of e.g.
bprm allocation leaves the reference unconsumed.  Your change
makes it consistent in that respect, but it means that in normal
case you are getting refcount higher by 1 than the mainline.
Does the mainline have an extra fput() *in* *normal* *case*?
I can easily believe in buggered cleanups on failure, but...
Has that code ever been tested?

	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.

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

[Tetsuo Handa]

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...

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

[Tetsuo Handa]

On 2020/03/13 18:46, Tetsuo Handa wrote:
> 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.
> 

No response from Alexei, but I think that 449325b52b7a6208 ("umh:
introduce fork_usermode_blob() helper") just did not realize that
opening a file for execution needs special handling (i.e. denying
write access) compared to opening a file for read or write.

Can we send this patch?

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

[Tetsuo Handa]

Since free_bprm(bprm) always calls allow_write_access(bprm->file) and
fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file()
must call deny_write_access(file) and get_file(file) if called from
do_execve_file() path. Otherwise, use-after-free access can happen at
fput(file) in fork_usermode_blob().

  general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC
  CPU: 3 PID: 4131 Comm: insmod Tainted: G           O      5.6.0-rc5+ #978
  Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019
  RIP: 0010:fork_usermode_blob+0xaa/0x190

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 449325b52b7a6208 ("umh: introduce fork_usermode_blob() helper")
Cc: <stable@vger.kernel.org> [4.18+]
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
---
 fs/exec.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..ded3fa368dc7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename,
 	check_unsafe_exec(bprm);
 	current->in_execve = 1;
 
-	if (!file)
+	if (!file) {
 		file = do_open_execat(fd, filename, flags);
-	retval = PTR_ERR(file);
-	if (IS_ERR(file))
-		goto out_unmark;
+		retval = PTR_ERR(file);
+		if (IS_ERR(file))
+			goto out_unmark;
+	} else {
+		retval = deny_write_access(file);
+		if (retval)
+			goto out_unmark;
+		get_file(file);
+	}
 
 	sched_exec();
 
-- 
2.18.2

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

[Andrew Morton]

On Fri, 27 Mar 2020 09:51:34 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Since free_bprm(bprm) always calls allow_write_access(bprm->file) and
> fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file()
> must call deny_write_access(file) and get_file(file) if called from
> do_execve_file() path. Otherwise, use-after-free access can happen at
> fput(file) in fork_usermode_blob().
> 
>   general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC
>   CPU: 3 PID: 4131 Comm: insmod Tainted: G           O      5.6.0-rc5+ #978
>   Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019
>   RIP: 0010:fork_usermode_blob+0xaa/0x190

This is rather old code - what casued this to be observed now?  Some
unusual userspace behaviour?

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

[Al Viro]

On Fri, Mar 27, 2020 at 09:51:34AM +0900, Tetsuo Handa wrote:

> diff --git a/fs/exec.c b/fs/exec.c
> index db17be51b112..ded3fa368dc7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename,
>  	check_unsafe_exec(bprm);
>  	current->in_execve = 1;
>  
> -	if (!file)
> +	if (!file) {
>  		file = do_open_execat(fd, filename, flags);
> -	retval = PTR_ERR(file);
> -	if (IS_ERR(file))
> -		goto out_unmark;
> +		retval = PTR_ERR(file);
> +		if (IS_ERR(file))
> +			goto out_unmark;
> +	} else {
> +		retval = deny_write_access(file);
> +		if (retval)
> +			goto out_unmark;
> +		get_file(file);
> +	}

I still don't like it.  The bug is real, but... *yeccchhhh*

First of all, this assignment to "file" is misguiding -
assignment to bprm->file would've been a lot easier to
follow.  Furthermore, the damn thing already has much
too confusing cleanup logics.

Why is
out:
        if (bprm->mm) {
                acct_arg_size(bprm, 0);
                mmput(bprm->mm);
        }
done on failure exit in this function and not in free_bprm(),
while dropping bprm->file is in free_bprm()?

Note that flush_old_exec() will zero bprm->mm (after it transfers
the damn thing into current->mm), so we are fine here.  And getting
rid of that thing in __do_execve_file() simplifies the logics
in there, especially if you take everything from this
        if (!file)
up to
        retval = exec_binprm(bprm);
into a new function.  All those goto out_unmark/goto out turn
into plain returns.  And in __do_execve_file() we are left with
        ....
        current->in_execve = 1;
	retval = this_new_helper(whatever it needs passed to it);
        current->fs->in_exec = 0;
        current->in_execve = 0;
	if (!retval) {
		/* execve succeeded */
		rseq_execve(current);
		acct_update_integrals(current);
		task_numa_free(current, false);
	}
out_free:
        free_bprm(bprm);
        kfree(pathbuf);
out_files:
        if (displaced)
                put_files_struct(displaced);
out_ret:
        if (filename)
                putname(filename);
        return retval;
which is a lot easier to follow.  Especially if we lift the logics
for freeing pathbuf into free_bprm() as well (will need a flag there,
for "does ->filename need to be freed?").  And I really wonder if
sched_exec() is called in the right place - what's special about the
point after opening the binary to be and setting bprm->file to what
we got?

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

[Tetsuo Handa]

On 2020/03/29 9:55, Andrew Morton wrote:
> On Fri, 27 Mar 2020 09:51:34 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
>> Since free_bprm(bprm) always calls allow_write_access(bprm->file) and
>> fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file()
>> must call deny_write_access(file) and get_file(file) if called from
>> do_execve_file() path. Otherwise, use-after-free access can happen at
>> fput(file) in fork_usermode_blob().
>>
>>   general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC
>>   CPU: 3 PID: 4131 Comm: insmod Tainted: G           O      5.6.0-rc5+ #978
>>   Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019
>>   RIP: 0010:fork_usermode_blob+0xaa/0x190
> 
> This is rather old code - what casued this to be observed now?  Some
> unusual userspace behaviour?

I'm attempting to fix a regression for TOMOYO caused by commit 51f39a1f0cea1cac
("syscalls: implement execveat() system call") in 3.19 that execve() request fails
if fd argument is not AT_FDCWD, for TOMOYO needs to re-calculate "requested pathname
using AT_SYMLINK_NOFOLLOW" from LSM hook. That regression was practically not a big
problem because 99%+ of execve() request used AT_FDCWD, for in general executing a
program involves opening external libraries which have to be accessible from mount
namespace where execve() was requested. But commit 449325b52b7a6208 ("umh: introduce
fork_usermode_blob() helper") in 4.18 was a rather unique change that allows file-less
execution of a program, which means that file-less execution request fails because
TOMOYO can never calculate "requested pathname using AT_SYMLINK_NOFOLLOW".

This patch itself does not fix the regression for TOMOYO. But this patch fixes a
memory corruption bug which should be applied regardless of the regression for TOMOYO.
Although Al does not like this patch, I'd like to keep this patch minimal so that
RedHat folks can easily backport this patch to RHEL 8 (which uses 4.18).