| Message ID | 20191023122350.2447-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,DISCO,EOAN] UBUNTU: SAUCE: shiftfs: drop CAP_SYS_RESOURCE from effective capabilities | expand |
On 10/23/19 5:23 AM, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1849483 > > Currently shiftfs allows to exceed project quota and reserved space on > e.g. ext2. See [1] and especially [2] for a bug report. This is very > much not what we want. Quotas and reserverd space settings set on the > host need to respected. The cause for this issue is overriding the > credentials with the superblock creator's credentials whenever we > perform operations such as fallocate() or writes while retaining > CAP_SYS_RESOURCE. > > The fix is to drop CAP_SYS_RESOURCE from the effective capability set > after we have made a copy of the superblock creator's credential at > superblock creation time. This very likely gives us more security than > we had before and the regression potential seems limited. I would like > to try this apporach first before coming up with something potentially > more sophisticated. I don't see why CAP_SYS_RESOURCE should become a > limiting factor in most use-cases. > > [1]: https://github.com/lxc/lxd/issues/6333 > [2]: https://github.com/lxc/lxd/issues/6333#issuecomment-545154838 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Seems reasonable and I saw from the Github issue that this and the s_maxbytes patch received positive test results. Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > --- > fs/shiftfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index ac22a5bf5b1f..890c01c7af25 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1951,6 +1951,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sb->s_flags |= SB_POSIXACL; > > if (sbinfo->mark) { > + struct cred *cred_tmp; > struct super_block *lower_sb = path.mnt->mnt_sb; > > /* to mark a mount point, must root wrt lower s_user_ns */ > @@ -2005,11 +2006,14 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sbinfo->passthrough_mark = sbinfo->passthrough; > } > > - sbinfo->creator_cred = prepare_creds(); > - if (!sbinfo->creator_cred) { > + cred_tmp = prepare_creds(); > + if (!cred_tmp) { > err = -ENOMEM; > goto out_put_path; > } > + /* Don't override disk quota limits or use reserved space. */ > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > + sbinfo->creator_cred = cred_tmp; > } else { > /* > * This leg executes if we're admin capable in the namespace, >
On 23.10.19 14:23, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1849483 > > Currently shiftfs allows to exceed project quota and reserved space on > e.g. ext2. See [1] and especially [2] for a bug report. This is very > much not what we want. Quotas and reserverd space settings set on the > host need to respected. The cause for this issue is overriding the > credentials with the superblock creator's credentials whenever we > perform operations such as fallocate() or writes while retaining > CAP_SYS_RESOURCE. > > The fix is to drop CAP_SYS_RESOURCE from the effective capability set > after we have made a copy of the superblock creator's credential at > superblock creation time. This very likely gives us more security than > we had before and the regression potential seems limited. I would like > to try this apporach first before coming up with something potentially > more sophisticated. I don't see why CAP_SYS_RESOURCE should become a > limiting factor in most use-cases. > > [1]: https://github.com/lxc/lxd/issues/6333 > [2]: https://github.com/lxc/lxd/issues/6333#issuecomment-545154838 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > fs/shiftfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index ac22a5bf5b1f..890c01c7af25 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1951,6 +1951,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sb->s_flags |= SB_POSIXACL; > > if (sbinfo->mark) { > + struct cred *cred_tmp; > struct super_block *lower_sb = path.mnt->mnt_sb; > > /* to mark a mount point, must root wrt lower s_user_ns */ > @@ -2005,11 +2006,14 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sbinfo->passthrough_mark = sbinfo->passthrough; > } > > - sbinfo->creator_cred = prepare_creds(); > - if (!sbinfo->creator_cred) { > + cred_tmp = prepare_creds(); > + if (!cred_tmp) { > err = -ENOMEM; > goto out_put_path; > } > + /* Don't override disk quota limits or use reserved space. */ > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > + sbinfo->creator_cred = cred_tmp; > } else { > /* > * This leg executes if we're admin capable in the namespace, >
On 2019-10-23 14:23:50 , Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1849483 > > Currently shiftfs allows to exceed project quota and reserved space on > e.g. ext2. See [1] and especially [2] for a bug report. This is very > much not what we want. Quotas and reserverd space settings set on the > host need to respected. The cause for this issue is overriding the > credentials with the superblock creator's credentials whenever we > perform operations such as fallocate() or writes while retaining > CAP_SYS_RESOURCE. > > The fix is to drop CAP_SYS_RESOURCE from the effective capability set > after we have made a copy of the superblock creator's credential at > superblock creation time. This very likely gives us more security than > we had before and the regression potential seems limited. I would like > to try this apporach first before coming up with something potentially > more sophisticated. I don't see why CAP_SYS_RESOURCE should become a > limiting factor in most use-cases. > > [1]: https://github.com/lxc/lxd/issues/6333 > [2]: https://github.com/lxc/lxd/issues/6333#issuecomment-545154838 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > fs/shiftfs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index ac22a5bf5b1f..890c01c7af25 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -1951,6 +1951,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sb->s_flags |= SB_POSIXACL; > > if (sbinfo->mark) { > + struct cred *cred_tmp; > struct super_block *lower_sb = path.mnt->mnt_sb; > > /* to mark a mount point, must root wrt lower s_user_ns */ > @@ -2005,11 +2006,14 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sbinfo->passthrough_mark = sbinfo->passthrough; > } > > - sbinfo->creator_cred = prepare_creds(); > - if (!sbinfo->creator_cred) { > + cred_tmp = prepare_creds(); > + if (!cred_tmp) { > err = -ENOMEM; > goto out_put_path; > } > + /* Don't override disk quota limits or use reserved space. */ > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > + sbinfo->creator_cred = cred_tmp; > } else { > /* > * This leg executes if we're admin capable in the namespace, > -- > 2.23.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Wed, Oct 23, 2019 at 02:23:50PM +0200, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1849483 > > Currently shiftfs allows to exceed project quota and reserved space on > e.g. ext2. See [1] and especially [2] for a bug report. This is very > much not what we want. Quotas and reserverd space settings set on the > host need to respected. The cause for this issue is overriding the > credentials with the superblock creator's credentials whenever we > perform operations such as fallocate() or writes while retaining > CAP_SYS_RESOURCE. > > The fix is to drop CAP_SYS_RESOURCE from the effective capability set > after we have made a copy of the superblock creator's credential at > superblock creation time. This very likely gives us more security than > we had before and the regression potential seems limited. I would like > to try this apporach first before coming up with something potentially > more sophisticated. I don't see why CAP_SYS_RESOURCE should become a > limiting factor in most use-cases. > > [1]: https://github.com/lxc/lxd/issues/6333 > [2]: https://github.com/lxc/lxd/issues/6333#issuecomment-545154838 > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Applied to unstable/master, thanks!
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index ac22a5bf5b1f..890c01c7af25 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -1951,6 +1951,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, sb->s_flags |= SB_POSIXACL; if (sbinfo->mark) { + struct cred *cred_tmp; struct super_block *lower_sb = path.mnt->mnt_sb; /* to mark a mount point, must root wrt lower s_user_ns */ @@ -2005,11 +2006,14 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, sbinfo->passthrough_mark = sbinfo->passthrough; } - sbinfo->creator_cred = prepare_creds(); - if (!sbinfo->creator_cred) { + cred_tmp = prepare_creds(); + if (!cred_tmp) { err = -ENOMEM; goto out_put_path; } + /* Don't override disk quota limits or use reserved space. */ + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); + sbinfo->creator_cred = cred_tmp; } else { /* * This leg executes if we're admin capable in the namespace,
BugLink: https://bugs.launchpad.net/bugs/1849483 Currently shiftfs allows to exceed project quota and reserved space on e.g. ext2. See [1] and especially [2] for a bug report. This is very much not what we want. Quotas and reserverd space settings set on the host need to respected. The cause for this issue is overriding the credentials with the superblock creator's credentials whenever we perform operations such as fallocate() or writes while retaining CAP_SYS_RESOURCE. The fix is to drop CAP_SYS_RESOURCE from the effective capability set after we have made a copy of the superblock creator's credential at superblock creation time. This very likely gives us more security than we had before and the regression potential seems limited. I would like to try this apporach first before coming up with something potentially more sophisticated. I don't see why CAP_SYS_RESOURCE should become a limiting factor in most use-cases. [1]: https://github.com/lxc/lxd/issues/6333 [2]: https://github.com/lxc/lxd/issues/6333#issuecomment-545154838 Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)