Re: fs: NULL deref in atime_needs_update

["Mickaël Salaün" <mic@digikod.net>]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]