linux-kernel - Re: fs: NULL deref in atime_needs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56C8CBF4.9020207@digikod.net>
Date:	Sat, 20 Feb 2016 21:26:28 +0100
From:	Mickaël Salaün <mic@...ikod.net>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Dmitry Vyukov <dvyukov@...gle.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: fs: NULL deref in atime_needs_update


On 20/02/2016 18:10, Al Viro wrote:
> On Sat, Feb 20, 2016 at 02:25:40PM +0100, Mickaël Salaün wrote:
> 
>> I think the bug may be somewhere in the nd->depth handling (when its value is 0) in fs/namei.c:get_link(): struct saved *last = nd->stack + nd->depth - 1
> 
> Getting there with nd->depth == 0 would certainly be a bug - it would mean
> that we got there without should_follow_link() having returned 1.
> 
> In case of open() it would be "do_last() has returned positive without
> should_follow_link() having returned 1".

OK, the do_last() return value was the origin of my bug in fs/namei.c:path_openat(): while (!(error = link_path_walk(s, nd)) && (error = do_last(nd, file, op, &opened)) > 0)

> 
> <looks>
> 
> OK, there are several places where we rely on not getting bogus return values
> - inode_permission() should not return positives, neither should vfs_open(),
> security_path_truncate() and notify_change().
> 
> Other similar "handle the last component" functions are guaranteed to
> never return positives other than directly from should_follow_link(), so
> they are OK.
> 
> IIRC, you used LSM to inject a positive value to inode_permission(), right?

Yes, my test hook was wrong because it returned 1 (instead of -EPERM or -EACCES) which is an invalid return value.

> diff --git a/fs/namei.c b/fs/namei.c
> index f624d13..e30deef 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3273,6 +3273,10 @@ opened:
>  			goto exit_fput;
>  	}
>  out:
> +	if (unlikely(error > 0)) {
> +		WARN_ON(1);
> +		error = -EINVAL;
> +	}
>  	if (got_write)
>  		mnt_drop_write(nd->path.mnt);
>  	path_put(&save_parent);
> 

I think your warning patch should be upstreamed to detect such cases :)

Thanks,
 Mickaël


Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)