Re: [PATCH 0/1] selinux: Add xfs quota command types

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Feb 24, 2020 at 04:41:17PM +0000, Richard Haines wrote:
> On Sat, 2020-02-22 at 14:49 -0500, Paul Moore wrote:
> > On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig <hch@infradead.org
> > > wrote:
> > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley wrote:
> > > > > The dquot_* routines are not generic quota code, but a specific
> > > > > implementation used by most non-XFS file systems.  So if there
> > > > > is a bug
> > > > > it is that the security call is not in the generic dispatch
> > > > > code.
> > > > 
> > > > Hmm...any reason the security hook call couldn't be taken to
> > > > quota_quotaon()?
> > > 
> > > I haven't touched the quota code for a while, but yes, the existing
> > > calls should move to the quota_* routines in
> > > fs/quota/quota.c.  Note
> > > that you still need to add checks, e.g. for Q_XSETQLIM.
> > 
> > Who wanted to submit a patch for this?  Christoph were you planning
> > on
> > fixing this?  If not, Richard, do you want to give it a try?
> > 
> 
> I've had a go at this and found I can (almost) get it working. I've
> attached a sample patch just in case anyone is interested.

Almost?  Are you talking about the weird looking dentry?

> However if the calls do need to move to fs/quota/quota.c, then I think
> the xfs team are best placed to do this (I've had my playtime).
> 
> 

> diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
> index 38669e827..c02cf9a63 100644
> --- a/fs/xfs/xfs_quotaops.c
> +++ b/fs/xfs/xfs_quotaops.c
> @@ -14,7 +14,7 @@
>  #include "xfs_trans.h"
>  #include "xfs_icache.h"
>  #include "xfs_qm.h"
> -
> +#include <linux/security.h>
>  
>  static void
>  xfs_qm_fill_state(
> @@ -161,11 +161,15 @@ xfs_quota_enable(
>  	unsigned int		uflags)
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
> +	struct dentry		*dentry = mp->m_super->s_root;
>  
>  	if (sb_rdonly(sb))
>  		return -EROFS;
>  	if (!XFS_IS_QUOTA_RUNNING(mp))
>  		return -ENOSYS;
> +	/* Emulates dquot_quota_on() Labeled correctly */
> +	if (!security_quota_on(dentry))
> +		return -EACCES;
>  
>  	return xfs_qm_scall_quotaon(mp, xfs_quota_flags(uflags));
>  }
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 2094386af..59855d060 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -39,6 +39,7 @@
>  #include <linux/magic.h>
>  #include <linux/fs_context.h>
>  #include <linux/fs_parser.h>
> +#include <linux/security.h>
>  
>  static const struct super_operations xfs_super_operations;
>  
> @@ -1507,6 +1508,22 @@ xfs_fc_fill_super(
>  		goto out_unmount;
>  	}
>  
> +	/*
> +	 * Emulates dquot_quota_on_mount() and works, however the dentry
> +	 * is unlabeled:
> +	 *    allow test_filesystem_t unlabeled_t:dir { quotaon };
> +	 */
> +	struct xfs_mount	*mpx = XFS_M(sb);
> +	struct dentry		*dentry = mpx->m_super->s_root;
> +	if (XFS_IS_QUOTA_RUNNING(mp)) {
> +		error = security_quota_on(dentry);

Er... I'm not sure if this is doing what you really want it to?  Mostly
because I don't know what the quota security hooks are supposed to
activate against? :)

The other three filesystems (ext4/f2fs/reiserfs) keep their quota files
linked off the root directory, and dquot_quota_on_mount looks up the
dentry for a root directory quota file and feeds that into
security_quota_on.

This patch calls it on XFS' root directory itself (quota inodes are not
linked in the directory tree at all on xfs), which I guess is the best
you can do, but might not be what the security hooks is expecting.

<shrug> Apologies for my unfamiliarity.

--D

> +		if (error) {
> +			dput(sb->s_root);
> +			sb->s_root = NULL;
> +			goto out_unmount;
> +		}
> +	}
> +
>  	return 0;
>  
>   out_filestream_unmount:
> diff --git a/security/security.c b/security/security.c
> index db7b574c9..ac0cc9872 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -770,6 +770,7 @@ int security_quota_on(struct dentry *dentry)
>  {
>  	return call_int_hook(quota_on, 0, dentry);
>  }
> +EXPORT_SYMBOL(security_quota_on);
>  
>  int security_syslog(int type)
>  {