On Tue, Sep 29, 2020 at 9:31 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Tue, Sep 29, 2020 at 8:54 AM Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> > On 9/27/20 5:42 AM, rentianyue@xxxxxxxxxxxxx wrote:
> > > From: Tianyue Ren <rentianyue@xxxxxxxxxx>
> > >
> > > Fix to initialize isec->class with SECINITSID_UNLABELED other
> > > than the from the xattr label when then dentry is NULL when
> > > the filesystem is remounted before the policy loading.
> >
> > Looks like this was broken by commit
> > 9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 ("selinux: Convert isec->lock
> > into a spinlock").
>
> It appears that the broken commit assumed (wrongly) that isec->sid is
> 0 initially, sets sid = isec->sid, and then in the out: path, if (!sid
> || rc) it sets isec->initialized to LABEL_INVALID. In fact, isec->sid
> is SECINITSID_UNLABELED initially upon selinux_inode_alloc_security(),
> so that !sid test never evaluates to true. And changing it to compare
> with SECINITSID_UNLABELED wouldn't be safe either since it is possible
> to end up with SECINITSID_UNLABELED without it being invalid. I think
> your fix resolves the issue with ensuring that we retry upon
> subsequent attempts to access the inode but we should likely fix up
> this code.
Beyond the patch that has already been posted, I think the fix/clean
up is probably just to change the "!sid || rc" conditional in the
"out" jump target to simply "rc". All of the code above that appears
to set "rc" correctly on error, which is really the only time (beyond
the posted patch) that we would need to set "isec->initizalized" to
"LABEL_INVALID".
--
paul moore
www.paul-moore.com
