On Wed, Jul 10, 2024 at 8:23 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > >
> > > I didn't find specific issues with Landlock's code except the extra
> > > check in hook_inode_free_security(). It looks like inode->i_security is
> > > a dangling pointer, leading to UAF.
> > >
> > > Reading security_inode_free() comments, two things looks weird to me:
> > >
> > > > /**
> > > > * security_inode_free() - Free an inode's LSM blob
> > > > * @inode: the inode
> > > > *
> > > > * Deallocate the inode security structure and set @inode->i_security to NULL.
> > >
> > > I don't see where i_security is set to NULL.
> >
> > The function header comments are known to be a bit suspect, a side
> > effect of being detached from the functions for many years, this may
> > be one of those cases. I tried to fix up the really awful ones when I
> > moved the comments back, back I didn't have time to go through each
> > one in detail. Patches to correct the function header comments are
> > welcome and encouraged! :)
> >
> > > > */
> > > > void security_inode_free(struct inode *inode)
> > > > {
> > >
> > > Shouldn't we add this check here?
> > > if (!inode->i_security)
> > > return;
> >
> > Unless I'm remembering something wrong, I believe we *should* always
> > have a valid i_security pointer each time we are called, if not
> > something has gone wrong, e.g. the security_inode_free() hook is no
> > longer being called from the right place. If we add a NULL check, we
> > should probably have a WARN_ON(), pr_err(), or something similar to
> > put some spew on the console/logs.
> >
> > All that said, it would be good to hear some confirmation from the VFS
> > folks that the security_inode_free() hook is located in a spot such
> > that once it exits it's current RCU critical section it is safe to
> > release the associated LSM state.
> >
> > It's also worth mentioning that while we always allocate i_security in
> > security_inode_alloc() right now, I can see a world where we allocate
> > the i_security field based on need using the lsm_blob_size info (maybe
> > that works today? not sure how kmem_cache handled 0 length blobs?).
> > The result is that there might be a legitimate case where i_security
> > is NULL, yet we still want to call into the LSM using the
> > inode_free_security() implementation hook.
>
> Looking at existing LSM implementations, even if some helpers (e.g.
> selinux_inode) return NULL if inode->i_security is NULL, this may not be
> handled by the callers. For instance, SELinux always dereferences the
> blob pointer in the security_inode_permission() hook.
Since SELinux requires space in inode->i_security there should never
be a case where SELinux is enabled and we don't allocate a blob for
inode->i_security.
> Shouldn't we remove all inode->i_security checks and assume it is always
> set? This is currently the case anyway, but it would be clearer this
> way and avoid false sense of security (with useless checks).
It would be interesting to draft a patch to do that and make sure
everything still works; it *should*, but I'd want to see that change
get some good testing :)
Keep in mind, this still doesn't mean that an LSM is required to use
any space in inode->i_security if it wants to use the inode hooks.
--
paul-moore.com
