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