Dave Chinner [off-list ref] writes:
On Tue, Jul 05, 2016 at 10:34:49AM -0500, Eric W. Biederman wrote:
quoted
The more I think of it the more I think that sounds like wisdom.
Dropping this patch and replacing it by one that just does:
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index d8fb0fd3ff6f..9c9890fe18b7 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2273,6 +2273,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
error = -EINVAL;
goto out_fmt;
}
+ /* Filesystems outside of init_user_ns not yet supported */
+ if (sb->s_user_ns != &init_user_ns) {
+ error = -EINVAL;
+ goto out_fmt;
+ }
/* Usage always has to be set... */
if (!(flags & DQUOT_USAGE_ENABLED)) {
error = -EINVAL;
seems a lot more appropriate at this point. That is enough to give a
great big hint there is something that needs to be done but won't
embrittle the code with untested corner cases.
You'll need to propagate that to all filesystems that have their own
quota implemenation, too.
All of the filesytems that have their own quota implementations omit the
flag FS_USERNS_MOUNT in fs_flags in struct filesystem so they are
protected.
The above change is extending to the generic implementation of quota
files the same protection that is already enjoyed by filesystems in
general. Recognition that being attacked by malicious users is a
difficult thing to defend against, and not enabling the code until there
is a reasonable certainty the code is robust against maliciuos attack
and everyone involved with maintaining the code is comfortable about the
situtation.
I have every intention of keeping all of the changes to the generic
parts of the quota layer so that filesystems who wish to implement
unprivileged mounts and implement quotas may proceed if they so desire.
Eric
p.s. I expect the the generic quota implementation is simple enough
that it is not particularly suseptible to problems caused by malicious
data. But I don't currently care enough to verify and test that
assumption so this is very much the wrong time for me to be enabling the
feature.
On Tue, Jul 05, 2016 at 04:28:53PM -0500, Eric W. Biederman wrote:
Dave Chinner [off-list ref] writes:
quoted
On Tue, Jul 05, 2016 at 10:34:49AM -0500, Eric W. Biederman wrote:
quoted
The more I think of it the more I think that sounds like wisdom.
Dropping this patch and replacing it by one that just does:
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index d8fb0fd3ff6f..9c9890fe18b7 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2273,6 +2273,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
error = -EINVAL;
goto out_fmt;
}
+ /* Filesystems outside of init_user_ns not yet supported */
+ if (sb->s_user_ns != &init_user_ns) {
+ error = -EINVAL;
+ goto out_fmt;
+ }
/* Usage always has to be set... */
if (!(flags & DQUOT_USAGE_ENABLED)) {
error = -EINVAL;
seems a lot more appropriate at this point. That is enough to give a
great big hint there is something that needs to be done but won't
embrittle the code with untested corner cases.
You'll need to propagate that to all filesystems that have their own
quota implemenation, too.
All of the filesytems that have their own quota implementations omit the
flag FS_USERNS_MOUNT in fs_flags in struct filesystem so they are
protected.
Which is the same situation currently for every filesystem that
supports quotas, regardless of the implementation infrastructure.
p.s. I expect the the generic quota implementation is simple enough
that it is not particularly suseptible to problems caused by malicious
data. But I don't currently care enough to verify and test that
assumption so this is very much the wrong time for me to be enabling the
feature.
All the more reason you should be adding the same guard to all the
other filesystems....
All i'm asking you to do is to make this check in a way that all
filesystems that implement quotas will execute it. Don't leave
landmines with security implications around - make sure all
filesystems have the same protections.
-Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
On Wed 06-07-16 16:35:04, Dave Chinner wrote:
On Tue, Jul 05, 2016 at 04:28:53PM -0500, Eric W. Biederman wrote:
quoted
Dave Chinner [off-list ref] writes:
quoted
On Tue, Jul 05, 2016 at 10:34:49AM -0500, Eric W. Biederman wrote:
quoted
The more I think of it the more I think that sounds like wisdom.
Dropping this patch and replacing it by one that just does:
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index d8fb0fd3ff6f..9c9890fe18b7 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2273,6 +2273,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
error = -EINVAL;
goto out_fmt;
}
+ /* Filesystems outside of init_user_ns not yet supported */
+ if (sb->s_user_ns != &init_user_ns) {
+ error = -EINVAL;
+ goto out_fmt;
+ }
/* Usage always has to be set... */
if (!(flags & DQUOT_USAGE_ENABLED)) {
error = -EINVAL;
seems a lot more appropriate at this point. That is enough to give a
great big hint there is something that needs to be done but won't
embrittle the code with untested corner cases.
You'll need to propagate that to all filesystems that have their own
quota implemenation, too.
All of the filesytems that have their own quota implementations omit the
flag FS_USERNS_MOUNT in fs_flags in struct filesystem so they are
protected.
Which is the same situation currently for every filesystem that
supports quotas, regardless of the implementation infrastructure.
quoted
p.s. I expect the the generic quota implementation is simple enough
that it is not particularly suseptible to problems caused by malicious
data. But I don't currently care enough to verify and test that
assumption so this is very much the wrong time for me to be enabling the
feature.
All the more reason you should be adding the same guard to all the
other filesystems....
All i'm asking you to do is to make this check in a way that all
filesystems that implement quotas will execute it. Don't leave
landmines with security implications around - make sure all
filesystems have the same protections.
Well, I'm not sure I follow you here. VFS quotas are a generic code used by
a few filesystems. So I can imagine that someone would decide to enable
FS_USERNS_MOUNT for one of those filesystems without thinking about quotas
and then Eric's check would trigger and possibly save use from some
problems.
When someone decides to enable FS_USERNS_MOUNT for XFS, he will have
presumably made sure all parts of XFS are safe, including its quota
implementation.
I don't want to stop you or Eric in adding an extra check in XFS, I just
have hard time to see how that check would trigger and how XFS quota is
different from other XFS parts...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR