On Fri, 25 Feb 2022 at 01:25, Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Thu, Feb 17, 2022 at 9:35 AM Christian Göttsche > <cgzones@xxxxxxxxxxxxxx> wrote: > > > > Log the anonymous inode class name in the security hook > > inode_init_security_anon. This name is the key for name based type > > transitions on the anon_inode security class on creation. Example: > > > > type=AVC msg=audit(02/16/22 22:02:50.585:216) : avc: granted { create } for pid=2136 comm=mariadbd anonclass="[io_uring]" dev="anon_inodefs" ino=6871 scontext=system_u:system_r:mysqld_t:s0 tcontext=system_u:system_r:mysqld_iouring_t:s0 tclass=anon_inode > > > > Add a new LSM audit data type holding the inode and the class name. > > > > Also warn if the security hook gets called with no name set; currently > > the only caller fs/anon_inodes.c:anon_inode_make_secure_inode() passes > > one. > > > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> > > --- > > include/linux/lsm_audit.h | 5 +++++ > > security/lsm_audit.c | 21 +++++++++++++++++++++ > > security/selinux/hooks.c | 7 +++++-- > > 3 files changed, 31 insertions(+), 2 deletions(-) > > ... > > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > > index 17d02eda9538..8135a88d6d82 100644 > > --- a/include/linux/lsm_audit.h > > +++ b/include/linux/lsm_audit.h > > @@ -76,6 +76,7 @@ struct common_audit_data { > > #define LSM_AUDIT_DATA_IBENDPORT 14 > > #define LSM_AUDIT_DATA_LOCKDOWN 15 > > #define LSM_AUDIT_DATA_NOTIFICATION 16 > > +#define LSM_AUDIT_DATA_ANONINODE 17 > > union { > > struct path path; > > struct dentry *dentry; > > @@ -96,6 +97,10 @@ struct common_audit_data { > > struct lsm_ibpkey_audit *ibpkey; > > struct lsm_ibendport_audit *ibendport; > > int reason; > > + struct { > > + struct inode *inode; > > + const char *anonclass; > > + } anoninode_struct; > > } u; > > /* this union contains LSM specific data */ > > union { > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > > index 1897cbf6fc69..5545fed35539 100644 > > --- a/security/lsm_audit.c > > +++ b/security/lsm_audit.c > > @@ -433,6 +433,27 @@ static void dump_common_audit_data(struct audit_buffer *ab, > > audit_log_format(ab, " lockdown_reason=\"%s\"", > > lockdown_reasons[a->u.reason]); > > break; > > + case LSM_AUDIT_DATA_ANONINODE: { > > + struct dentry *dentry; > > + struct inode *inode; > > + > > + rcu_read_lock(); > > + inode = a->u.anoninode_struct.inode; > > + dentry = d_find_alias_rcu(inode); > > + if (dentry) { > > + audit_log_format(ab, " name="); > > + spin_lock(&dentry->d_lock); > > + audit_log_untrustedstring(ab, dentry->d_name.name); > > + spin_unlock(&dentry->d_lock); > > + } > > I'm not sure we are ever going to get a useful dentry name for > anonymous inodes, I think we can probably drop this. The "anonclass=" > field will likely be much more interesting and helpful. > > > + audit_log_format(ab, " anonclass="); > > + audit_log_untrustedstring(ab, a->u.anoninode_struct.anonclass); > > + audit_log_format(ab, " dev="); > > + audit_log_untrustedstring(ab, inode->i_sb->s_id); > > I'm pretty sure this is always going to end up being "anon_inodefs" > and thus not very useful. > > > + audit_log_format(ab, " ino=%lu", inode->i_ino); > > Similarly, I'm not sure how useful the inode number is in practice. > I've never tried, but can a user lookup an anonymous inode via the > inode number? > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index dafabb4dcc64..19c831d94d9b 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2932,6 +2932,8 @@ static int selinux_inode_init_security_anon(struct inode *inode, > > if (unlikely(!selinux_initialized(&selinux_state))) > > return 0; > > > > + WARN_ON(!name); > > + > > isec = selinux_inode(inode); > > > > /* > > @@ -2965,8 +2967,9 @@ static int selinux_inode_init_security_anon(struct inode *inode, > > * allowed to actually create this type of anonymous inode. > > */ > > > > - ad.type = LSM_AUDIT_DATA_INODE; > > - ad.u.inode = inode; > > + ad.type = LSM_AUDIT_DATA_ANONINODE; > > + ad.u.anoninode_struct.inode = inode; > > + ad.u.anoninode_struct.anonclass = name ? (const char *)name->name : "unknown(null)"; > > This doesn't seem to match well with the newly added WARN_ON() > assertion above. I would suggest dropping the WARN_ON() assertion as > security_transition_sid() can already handle that safely, and leaving > the tertiary statement above; however I think we should probably > change the anonclass string to "?" as that is the common unset field > value used by audit. Is the hook inode_init_security_anon expected to be called with an empty name though? The condition was just a fallback to not crash the kernel. (Dropped in v2.) > > -- > paul-moore.com