On Wed, Mar 15, 2023 at 1:37 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Wed, Mar 15, 2023 at 5:33 AM Stephen Smalley > <stephen.smalley.work@xxxxxxxxx> wrote: > > > > We are already taking the isec->lock (or otherwise have exclusive > > access to a newly initialized isec) in all the places where we are > > updating the isec->avd and isec->avdsid. The issue is not the updates > > but rather the reads in inode_has_perm() and > > selinux_inode_permission(). > > Right. > > And that is always going to be unordered, in the sense that you will > get "one or the other" value whether you have strict locking or not. > > So even with a spinlock around the actual low-level selinux data reads > and writes, there is only "data consistency", not any actual > *ordering*. The pathname lookup itself is simply not ordered (and > cannot in any sane model be ordered) wrt somebody else coming in and > changing any selinux rules. > > So I don't think this is even worth worrying about. There is no > ordering, because no ordering can possibly exist. > > The only thing that can matter is consistency: any *individual* > security decision should either get the old rules or the new rules > (never a mix of the two), but either of those is fine - and as you > traverse a whole pathname and do multiple different security decisions > for each path component (and for the final open), you *will* get a > mixture of old and new if the rules are updated concurrently. > > I don't think this is a problem, and I don't even think it's fixable > (sure, in theory, we could do some big sequence number lock or > similar, but no way do we actually want to do that for path lookup in > reality). So my primary concern is wrt consistency of the (isec->task_sid, isec->avdsid, isec->avd) triple. In particular, if we have two or more tasks with different SIDs accessing the same inode, then selinux_file_open() is going to update those values on each open(), and inode_has_perm() and selinux_inode_permission() may see inconsistent states with a mixture of the old and new SID pair and access vectors that will result in permission being incorrectly allowed or denied. I'm not sure how to safely prevent this in a manner that doesn't impose too much overhead on inode_has_perm() and selinux_inode_permission().
