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