On Wed, May 27, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@xxxxxxxxxx> wrote: > > On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > I might be missing something, but couldn't you achieve the same by > > > simply adding something like this in here: > > > > > > if (!selinux_initialized(&selinux_state)) > > > return -EOPNOTSUPP; > > > > > > (Or by adding it to the condition above.) > > > > > > Then you should hit this condition here and be all set: > > > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 > > > > Hi Ondrej, > > > > Yes, that looks promising. Two questions with that approach: > > > > 1. Is there a concern here with transiently returning -EOPNOTSUPP even > > if the SELinux LSM does technically support the inode_getsecurity > > hook? I'm thinking of potential corner-cases down the road where > > somehow this knowledge is cached. > > I would hope not. I don't think it's likely this would be cached, > since it would require a guarantee from all LSMs that they won't flip > from -EOPNOTSUPP to something else... That would be error-prone IMHO. > > > > > 2. The selinux_inode_getsecurity hook today does somewhat handle the > > uninitialized case. It ends up here: > > > > https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322. > > > > Specifically, it has support for initial SIDs. The patch I wrote > > purposely tries to allow falling back to that logic. Is there a > > concern with short-circuiting this logic even if the inode SID somehow > > isn't SECINITSID_UNLABELED? > > Oh, right, so that's what I missed :) I'll have to defer to Stephen on > whether this is a concern... Obviously we lose the previous behavior > of returning the initial SID strings via getxattr(), but I'm not sure > if that's significant. Since those strings obviously aren't real > contexts, it seems they only serve an informational purpose. > > Anyway, I looked at the original patch again and it generally looks > sane. I don't like the fact that we need to call back to > __vfs_getxattr() in yet another place, but it makes sense since > security_inode_getsecurity() is basically overriding it. So I leave it > on Stephen or Paul to decide which is better. I think Ondrej's suggested approach is better. I don't think it is a concern.
