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

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

[Richard Haines]

Added these quota command types for SELinux checks on XFS quotas. I picked
those I thought useful. The selinux-testsuite will have tests for these
permission checks on XFS.

One point to note: XFS does not call dquot_quota_on() to trigger
security_quota_on(), therefore the 'file quotaon' permission is not tested
for SELinux

Richard Haines (1):
  selinux: Add xfs quota command types

 security/selinux/hooks.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.24.1

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

[Richard Haines]

Add Q_XQUOTAOFF, Q_XQUOTAON and Q_XSETQLIM to trigger filesystem quotamod
permission check.

Add Q_XGETQUOTA, Q_XGETQSTAT, Q_XGETQSTATV and Q_XGETNEXTQUOTA to trigger
filesystem quotaget permission check.

Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
---
 security/selinux/hooks.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 46a8f3e7d..974228313 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2145,11 +2145,18 @@ static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
 	case Q_QUOTAOFF:
 	case Q_SETINFO:
 	case Q_SETQUOTA:
+	case Q_XQUOTAOFF:
+	case Q_XQUOTAON:
+	case Q_XSETQLIM:
 		rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAMOD, NULL);
 		break;
 	case Q_GETFMT:
 	case Q_GETINFO:
 	case Q_GETQUOTA:
+	case Q_XGETQUOTA:
+	case Q_XGETQSTAT:
+	case Q_XGETQSTATV:
+	case Q_XGETNEXTQUOTA:
 		rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAGET, NULL);
 		break;
 	default:
-- 
2.24.1

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

[Christoph Hellwig]

On Thu, Feb 20, 2020 at 03:32:34PM +0000, Richard Haines wrote:
> Add Q_XQUOTAOFF, Q_XQUOTAON and Q_XSETQLIM to trigger filesystem quotamod
> permission check.
> 
> Add Q_XGETQUOTA, Q_XGETQSTAT, Q_XGETQSTATV and Q_XGETNEXTQUOTA to trigger
> filesystem quotaget permission check.
> 
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 03:32:33PM +0000, Richard Haines wrote:
> Added these quota command types for SELinux checks on XFS quotas. I picked
> those I thought useful. The selinux-testsuite will have tests for these
> permission checks on XFS.
> 
> One point to note: XFS does not call dquot_quota_on() to trigger
> security_quota_on(), therefore the 'file quotaon' permission is not tested
> for SELinux

Is that a feature or a bug? :)

(It sounds like a bug to me, but let's see if anyone complains...)

--D

> Richard Haines (1):
>   selinux: Add xfs quota command types
> 
>  security/selinux/hooks.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> -- 
> 2.24.1
> 

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

[Christoph Hellwig]

On Thu, Feb 20, 2020 at 07:57:31AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 03:32:33PM +0000, Richard Haines wrote:
> > Added these quota command types for SELinux checks on XFS quotas. I picked
> > those I thought useful. The selinux-testsuite will have tests for these
> > permission checks on XFS.
> > 
> > One point to note: XFS does not call dquot_quota_on() to trigger
> > security_quota_on(), therefore the 'file quotaon' permission is not tested
> > for SELinux
> 
> Is that a feature or a bug? :)
> 
> (It sounds like a bug to me, but let's see if anyone complains...)

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.

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

[Christoph Hellwig]

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.

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

[Paul Moore]

On Thu, Feb 20, 2020 at 10:32 AM Richard Haines
<richard_c_haines@btinternet.com> wrote:
>
> Add Q_XQUOTAOFF, Q_XQUOTAON and Q_XSETQLIM to trigger filesystem quotamod
> permission check.
>
> Add Q_XGETQUOTA, Q_XGETQSTAT, Q_XGETQSTATV and Q_XGETNEXTQUOTA to trigger
> filesystem quotaget permission check.
>
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
>  security/selinux/hooks.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Thanks Richard, I've merged this into selinux/next.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 46a8f3e7d..974228313 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2145,11 +2145,18 @@ static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
>         case Q_QUOTAOFF:
>         case Q_SETINFO:
>         case Q_SETQUOTA:
> +       case Q_XQUOTAOFF:
> +       case Q_XQUOTAON:
> +       case Q_XSETQLIM:
>                 rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAMOD, NULL);
>                 break;
>         case Q_GETFMT:
>         case Q_GETINFO:
>         case Q_GETQUOTA:
> +       case Q_XGETQUOTA:
> +       case Q_XGETQSTAT:
> +       case Q_XGETQSTATV:
> +       case Q_XGETNEXTQUOTA:
>                 rc = superblock_has_perm(cred, sb, FILESYSTEM__QUOTAGET, NULL);
>                 break;
>         default:
> --
> 2.24.1

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

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?

-- 
paul moore
www.paul-moore.com

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

[Richard Haines]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

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.

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).



[-- Attachment #2: security-xfs-quota.diff --]
[-- Type: text/x-patch, Size: 1899 bytes --]

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);
+		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)
 {

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

[Darrick J. Wong]

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)
>  {

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

[Richard Haines]

On Mon, 2020-02-24 at 20:13 -0800, Darrick J. Wong wrote:
> 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.

Thanks for your comments.
What I was trying to achieve is obtain a dentry for the root directory
to call the security hooks with. My reasoning was that as xfs quotas
are 'internal' (i.e. not files as in ext4 etc.), then using this as a
reference, I could check that the SELinux 'quotaon' permission was set
on this to allow quotas to be used (if set as mount options), if not
fail the mount.
Once mounted with quotas enabled, I could then use quotactl(2) to turn
them off/on, get stats etc. providing of course the mount directory
also had the SELinux quotamod and quotaget permissions as well (hence
the hook in xfs_quota_enable()).

As I don't know the xfs code at all, I was just experimenting to test
my theory, however the xfs dev team is better placed to resolve this as
its been suggested that the calls are moved to fs/quota/quota.c.


> 
> --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)
> >  {

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

[Darrick J. Wong]

On Tue, Feb 25, 2020 at 05:54:03PM +0000, Richard Haines wrote:
> On Mon, 2020-02-24 at 20:13 -0800, Darrick J. Wong wrote:
> > 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.
> 
> Thanks for your comments.
> What I was trying to achieve is obtain a dentry for the root directory
> to call the security hooks with. My reasoning was that as xfs quotas
> are 'internal' (i.e. not files as in ext4 etc.), then using this as a
> reference, I could check that the SELinux 'quotaon' permission was set
> on this to allow quotas to be used (if set as mount options), if not
> fail the mount.

Hmm, maybe let's back up a step here.  What's the purpose of being able
to set quota_on privileges on a file?  Is it so that sysadmins can
designate a particular file as a "quota" file and thereby prevent the
kernel from being tricked into loading some other file as the quota data
file?

I ask this because AFAICT in ext4, the "usrjquota=<path>" mount options
are for the old quota system wherein quotas are files that are managed
separately from the filesystem.

The new ext4 quota system links the quota inode pointers only from the
superblock, and it calls dquot_load_quota_inode, which doesn't seem to
have a security callout.  Seeing as there's no user-visible file for new
style quotas, I don't see why you'd need a selinux hook either.

I guess it could be generally useful to be able to be able to apply the
same quota_on labels to root directories of filesystems that have new
style quotas that one can apply to quota files on fses with old style
quotas... but (a) I don't see that being the case right now, and (b) if
you're trying to /add/ that functionality then yes it ought to be in
fs/quota/... and yes xfs will have to play catch up once it's there.

Is there already a precedent for setting quota_on labels to the root
dir?  It's entirely possible that I'm simply unaware of what's going on
in fs/quota/ because xfs mostly does its own things wrt quota.

--D

> Once mounted with quotas enabled, I could then use quotactl(2) to turn
> them off/on, get stats etc. providing of course the mount directory
> also had the SELinux quotamod and quotaget permissions as well (hence
> the hook in xfs_quota_enable()).
> 
> As I don't know the xfs code at all, I was just experimenting to test
> my theory, however the xfs dev team is better placed to resolve this as
> its been suggested that the calls are moved to fs/quota/quota.c.
> 
> 
> > 
> > --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)
> > >  {
> 

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

[Stephen Smalley]

On Tue, Feb 25, 2020 at 4:36 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> Hmm, maybe let's back up a step here.  What's the purpose of being able
> to set quota_on privileges on a file?  Is it so that sysadmins can
> designate a particular file as a "quota" file and thereby prevent the
> kernel from being tricked into loading some other file as the quota data
> file?

Yes.

> I ask this because AFAICT in ext4, the "usrjquota=<path>" mount options
> are for the old quota system wherein quotas are files that are managed
> separately from the filesystem.
>
> The new ext4 quota system links the quota inode pointers only from the
> superblock, and it calls dquot_load_quota_inode, which doesn't seem to
> have a security callout.  Seeing as there's no user-visible file for new
> style quotas, I don't see why you'd need a selinux hook either.
>
> I guess it could be generally useful to be able to be able to apply the
> same quota_on labels to root directories of filesystems that have new
> style quotas that one can apply to quota files on fses with old style
> quotas... but (a) I don't see that being the case right now, and (b) if
> you're trying to /add/ that functionality then yes it ought to be in
> fs/quota/... and yes xfs will have to play catch up once it's there.
>
> Is there already a precedent for setting quota_on labels to the root
> dir?  It's entirely possible that I'm simply unaware of what's going on
> in fs/quota/ because xfs mostly does its own things wrt quota.

Ok, in that case I'd say we don't need the hook to be called at all in
the cases where
the quota inode is private to the superblock and not directly exposed
to userspace.
So no change required to xfs or fs/quota and we can skip/ignore the test when
not using usrquota=...