Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338793

Re: fs: NULL deref in atime_needs_update

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-17 02:00 +0100
  Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-19 20:40 +0100
    Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 04:30 +0100
      Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 05:00 +0100
        Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-20 17:00 +0100
          Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 18:20 +0100
            Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-20 22:00 +0100
            Re: fs: NULL deref in atime_needs_update Mickaël Salaün <mic@digikod.net> - 2016-02-20 22:10 +0100
            Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-22 12:30 +0100
              Re: fs: NULL deref in atime_needs_update Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-22 18:30 +0100
      Re: fs: NULL deref in atime_needs_update Dmitry Vyukov <dvyukov@google.com> - 2016-02-20 11:40 +0100

csiph-web