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. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel, Red Hat, Inc.
