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