From mboxrd@z Thu Jan 1 00:00:00 1970 From: NeilBrown Subject: Re: [PATCH 04/13] security/selinux: check for LOOKUP_RCU in _follow_link. Date: Fri, 20 Mar 2015 15:39:30 +1100 Message-ID: <20150320153930.1f180e06@notabene.brown> References: <20150316043602.23648.52734.stgit@notabene.brown> <20150316044319.23648.82820.stgit@notabene.brown> <20150316210035.GC29656@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/AAsgRaL=ZVzE2C8Ip=QExq0"; protocol="application/pgp-signature" Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Al Viro Return-path: In-Reply-To: <20150316210035.GC29656@ZenIV.linux.org.uk> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org --Sig_/AAsgRaL=ZVzE2C8Ip=QExq0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Mon, 16 Mar 2015 21:00:35 +0000 Al Viro wrote: > On Mon, Mar 16, 2015 at 03:43:19PM +1100, NeilBrown wrote: > > Some of dentry_has_perm() is not rcu-safe, so if LOOKUP_RCU > > is set in selinux_inode_follow_link(), give up with > > -ECHILD. > >=20 > > It is possible that dentry_has_perm could sometimes complete > > in RCU more, in which case the flag could be propagated further > > down the stack... >=20 > It bloody well can. Expand it a bit and you'll see - the nastiness > comes from avc_audit() doing > return slow_avc_audit(ssid, tsid, tclass, > requested, audited, denied, result, > a, 0); > and passing that 0 to slow_avc_audit(). Pass it MAY_NOT_BLOCK instead > and it'll bugger off with -ECHILD in blocking case. >=20 > Call chain is dentry_has_perm -> inode_has_perm -> avc_has_perm -> avc_au= dit. > Expand those (including avc_audit()) and make slow_avc_audit() get > flags & LOOKUP_RCU ? MAY_NOT_BLOCK : 0. There is more to it than that. avc_has_perm calls avc_has_perm_noaudit which does: rcu_read_lock(); ... if (unlikely(!node)) { node =3D avc_compute_av(ssid, tsid, tclass, avd); } else ... ... rcu_read_unlock(); and avc_compute_av() does rcu_read_unlock(); security_compute_av(ssid, tsid, tclass, avd); rcu_read_lock(); (yes: unlock, and then lock). so avc_has_perm_noaudit needs to bail out of RCU-walk if node turns out to = be NULL. So I either add another 'flags' arg to that, or replace the current one whi= ch is unused .... or leave it as someone else's problem :-) NeilBrown --Sig_/AAsgRaL=ZVzE2C8Ip=QExq0 Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIVAwUBVQukgjnsnt1WYoG5AQJhbw/8DZMLXPwDW1xKhvPUPk9dK66KbLYkJS8w jkTdZDs/WIGmbjMMi8hcAkyorSLhy3S63zdAgakznrTx92dOJJXEWsynZwJk3ZX1 ggDb/EqVuN/2kaXAIA2ZpAvEuvlRB/Vi/rOYzTzh88Q2l2ysSx0Xq22DOGaGTbPn 3cd6nfAlYbWmLNdXWDMKrekofvK6jHihn9Ti3CRjjInzfAqvSvcWUjsvQW8wIxeP PzKTZna+5o/3BscOBkhFVmutJQ+pzc9kwKFP6v5fiQs6OR2a7VwPxTHVhzdORjQ4 sR1B1E3hFQlghUxaNbKOfrRzDseK1CohYleV5GpMBfiXpcinsTQmuiIvvrawQZ4+ n3ZUMk6gPCe39AH1k2GJKHpDCOrBFogiLKfNzPVtp6fttBYqw9tii2bKYmM+2wlH HV99Wzfiz2hrx4tPgi1/jBZ80MU00473KuVQQuoOvrWTSxUSVD2tBKLfafeWWAMc nfrbfJjUcdtJf9KMoxAwqUjqbS5T61Ee75ecYsDaTo0aGE3WxrZ+ymHvqjK+7XKT YuzHJUd5vz9mNYKNXr+Ct4ZHhKNjYSRW/BvFuho7gjTWhvQyDFRWCS2keKOEY3PK eqaPEVuckKoY2IwLKhURK7yL33Lpj3+nhdtgClmtFAqAvrPpQGZTzdhIwVCKYFq7 TtJwKlxqPdY= =1+UO -----END PGP SIGNATURE----- --Sig_/AAsgRaL=ZVzE2C8Ip=QExq0--