On Wed, Sep 11, 2019 at 5:28 PM Jonathan Lebon <jlebon@xxxxxxxxxx> wrote:
> On Tue, Aug 27, 2019 at 8:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > As I'm looking at this, I'm wondering why we don't just bail out early
> > if the policy isn't loaded? The context lookup and permission checks
> > later in the function are pretty much useless if the policy hasn't
> > been loaded (they are just going to return defaults/allow), I think
> > the only thing we would need to check would be
> > inode_owner_or_capable().
>
> Yes, I think you're correct. Though in that case, would it make sense
> to just do the inode_owner_or_capable() check once upfront instead?
>
> int selinux_inode_setxattr(...)
> {
>
> if (strcmp(name, XATTR_NAME_SELINUX)) {
> ...
> }
>
> if (!inode_owner_or_capable(inode)
> ...
>
> if (!selinux_state.initialized)
> return 0;
>
> if (sbsec & SBLABEL_MNT)
> ...
>
> ...
> }
>
> Hmm, though I guess it does change the behaviour slightly even in the
> initialized case by returning EPERM first where before we might've
> returned EOPNOTSUPP (I've seen userspace code which subtly relied on
> the order in which the kernel checks for error conditions). I'm happy
> to be conservative and go with your approach if you prefer.
Exactly. I suggested the approach I did because I was trying to avoid
changing the return behaviour; unless you can prove beyond a shadow of
a doubt that changing the return values doesn't break anything (that's
a pretty high bar), stick with the conservative approach.
--
paul moore
www.paul-moore.com
