On Wed, Mar 15, 2023 at 2:05 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > 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. Agreed. FWIW, I thought you were looking for a different approach for the writers in your original mail. Considering the multiple cache fields, and the consistency requirements, I think the only realistic solution would be to wrap them in their own struct/pointer referenced by the inode_security_struct and use RCU on the read side. Different readers would obviously have the potential for different views, but the cached values would at least be consistent for any given reader. The downside would be the allocations necessary when updating the cache, which I'm guessing would wipe out any gains from the cache ... but that's just a guess. > 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(). -- paul-moore.com
