From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: fs: NULL deref in atime_needs_update To: Al Viro References: <56C3B35E.6020109@digikod.net> <20160220032127.GA19926@ZenIV.linux.org.uk> <20160220035442.GE17997@ZenIV.linux.org.uk> <56C86954.6030101@digikod.net> <20160220171044.GH17997@ZenIV.linux.org.uk> Cc: Dmitry Vyukov , "linux-fsdevel@vger.kernel.org" , LKML , syzkaller , Kostya Serebryany , Alexander Potapenko , Sasha Levin From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= Message-ID: <56C8CBF4.9020207@digikod.net> Date: Sat, 20 Feb 2016 21:26:28 +0100 MIME-Version: 1.0 In-Reply-To: <20160220171044.GH17997@ZenIV.linux.org.uk> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="MMMiUB5pn40ivtA7R5XbGbgJoAXLAKTf8" Sender: linux-kernel-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --MMMiUB5pn40ivtA7R5XbGbgJoAXLAKTf8 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 20/02/2016 18:10, Al Viro wrote: > On Sat, Feb 20, 2016 at 02:25:40PM +0100, Micka=EBl Sala=FCn wrote: >=20 >> I think the bug may be somewhere in the nd->depth handling (when its v= alue is 0) in fs/namei.c:get_link(): struct saved *last =3D nd->stack + n= d->depth - 1 >=20 > Getting there with nd->depth =3D=3D 0 would certainly be a bug - it wou= ld mean > that we got there without should_follow_link() having returned 1. >=20 > 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:pat= h_openat(): while (!(error =3D link_path_walk(s, nd)) && (error =3D do_la= st(nd, file, op, &opened)) > 0) >=20 > >=20 > OK, there are several places where we rely on not getting bogus return = values > - inode_permission() should not return positives, neither should vfs_op= en(), > security_path_truncate() and notify_change(). >=20 > Other similar "handle the last component" functions are guaranteed to > never return positives other than directly from should_follow_link(), s= o > they are OK. >=20 > IIRC, you used LSM to inject a positive value to inode_permission(), ri= ght? 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 =3D -EINVAL; > + } > if (got_write) > mnt_drop_write(nd->path.mnt); > path_put(&save_parent); >=20 I think your warning patch should be upstreamed to detect such cases :) Thanks, Micka=EBl --MMMiUB5pn40ivtA7R5XbGbgJoAXLAKTf8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBCgAGBQJWyMv1AAoJECLe/t9zvWqV8bYIAJZDi4sl7bahohTqfhl2mB5B PHnqX/vnfackQPizD/LVN/up50KnX9ywg+NcJGki0MX+Rna05P3OWBDMt9sJDUCU qlEJ5Ed5tA+7Ns9m7Soono+q84BLFytmqtjTPMhU6nHI1VkaTQ2rvO7V9EQEvVaE uColyfy531dw2rd7kjuA23f8tVosA4QzDDJdLndIHqBBU6A1MjOba4EBatZKZh0r 3VumFXjq3ymm174FU9lnhU0jFyJ0ehHI+8rE+4Yxpt8FQaw/Twt7JTcR7Ttz6upN 0rG+WQW7imYjShLc3pa0KOvsP5Zedp66+MPXMhp2IMgYER/ZusetR7+4a1mOgdg= =fL1b -----END PGP SIGNATURE----- --MMMiUB5pn40ivtA7R5XbGbgJoAXLAKTf8--