From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757818AbcBTRKx (ORCPT ); Sat, 20 Feb 2016 12:10:53 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:58526 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751821AbcBTRKs (ORCPT ); Sat, 20 Feb 2016 12:10:48 -0500 Date: Sat, 20 Feb 2016 17:10:45 +0000 From: Al Viro To: =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= Cc: Dmitry Vyukov , "linux-fsdevel@vger.kernel.org" , LKML , syzkaller , Kostya Serebryany , Alexander Potapenko , Sasha Levin Subject: Re: fs: NULL deref in atime_needs_update Message-ID: <20160220171044.GH17997@ZenIV.linux.org.uk> References: <56C3B35E.6020109@digikod.net> <20160220032127.GA19926@ZenIV.linux.org.uk> <20160220035442.GE17997@ZenIV.linux.org.uk> <56C86954.6030101@digikod.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <56C86954.6030101@digikod.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, 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? Another way to trigger that would've been ->open() returning positive - a bug on *anything* since ->open() had been introduced in 0.95. Amount of harm would vary - e.g. 0.95 would simply have that positive number returned to userland, looking like successful open(2). With no new descriptor, of course... Short-term we probably want just if (unlikely(error > 0)) { WARN_ON(1); error = -EINVAL; } added right after out: in do_last(), try to trigger Dmitry's reproducers on it and then work back to the source of that thing *if* that's what's happening in his case. Yours almost certainly is just that. Longer-term... I'm not sure. Having a method that is supposed to return 0 or -E actually return positive is going to be a bad thing, no matter what, but "that bogus value gets passed to userland" is a lot more tolerable than "kernel memory corruption". do_last() calling conventions make it vulnerable to the latter, and as far as nd->stack underruns that's it, but I'm not sure we don't have other places where such bug in driver, etc. would translate into mess ;-/ OK, in any case, let's start with checking if Dmitry is seeing that and not something else. I still don't understand his stack traces - the fault address quoted in his first posting doesn't match the register values in the same trace, and there's also a possibility that it's an RCU-related crap. This should give a warning and prevent an oops if we are hitting a stack underrun on bogus positive from do_last(). Dmitry, could you try to build with delta below and run your reproducer(s)? 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);