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:
signature.asc
Description: OpenPGP digital signature
