| Message ID | 20181128030731.10288-1-lufq.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: drop file privileges in btrfs_clone_files | expand |
On 28.11.18 г. 5:07 ч., Lu Fengqi wrote: > The generic/513 tell that cloning into a file did not strip security > privileges (suid, capabilities) like a regular write would. > > Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > --- > The xfs and ocfs2 call generic_remap_file_range_prep to drop file > privileges, I'm not sure whether btrfs should do the same thing. Why do you think btrfs shouldn't do the same thing. Looking at remap_file_range_prep it seems that btrfs is missing a ton of checks that are useful i.e immutable files/aligned offsets etc. > > Any suggestion? > > fs/btrfs/ioctl.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 410c7e007ba8..bc33c480603b 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4312,6 +4312,10 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, > goto out_unlock; > } > > + ret = file_remove_privs(file); > + if (ret) > + goto out_unlock; > + > if (destoff > inode->i_size) { > ret = btrfs_cont_expand(inode, inode->i_size, destoff); > if (ret) >
On Wed, Nov 28, 2018 at 09:44:59AM +0200, Nikolay Borisov wrote: > > > On 28.11.18 г. 5:07 ч., Lu Fengqi wrote: > > The generic/513 tell that cloning into a file did not strip security > > privileges (suid, capabilities) like a regular write would. > > > > Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > > --- > > The xfs and ocfs2 call generic_remap_file_range_prep to drop file > > privileges, I'm not sure whether btrfs should do the same thing. > > Why do you think btrfs shouldn't do the same thing. Looking at > remap_file_range_prep it seems that btrfs is missing a ton of checks > that are useful i.e immutable files/aligned offsets etc. Any chance we could move btrfs over to use remap_file_range_prep so that all file systems share the exact same checks?
On 28.11.18 г. 9:46 ч., Christoph Hellwig wrote: > On Wed, Nov 28, 2018 at 09:44:59AM +0200, Nikolay Borisov wrote: >> >> >> On 28.11.18 г. 5:07 ч., Lu Fengqi wrote: >>> The generic/513 tell that cloning into a file did not strip security >>> privileges (suid, capabilities) like a regular write would. >>> >>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>> --- >>> The xfs and ocfs2 call generic_remap_file_range_prep to drop file >>> privileges, I'm not sure whether btrfs should do the same thing. >> >> Why do you think btrfs shouldn't do the same thing. Looking at >> remap_file_range_prep it seems that btrfs is missing a ton of checks >> that are useful i.e immutable files/aligned offsets etc. > > Any chance we could move btrfs over to use remap_file_range_prep so that > all file systems share the exact same checks? I'm not very familiar with the, Filipe is more familiar so adding to CC. But IMO we should do that provided there are no blockers. Filipe, what do you think, is it feasible? >
On Wed, Nov 28, 2018 at 09:48:07AM +0200, Nikolay Borisov wrote: > > >On 28.11.18 г. 9:46 ч., Christoph Hellwig wrote: >> On Wed, Nov 28, 2018 at 09:44:59AM +0200, Nikolay Borisov wrote: >>> >>> >>> On 28.11.18 г. 5:07 ч., Lu Fengqi wrote: >>>> The generic/513 tell that cloning into a file did not strip security >>>> privileges (suid, capabilities) like a regular write would. >>>> >>>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>>> --- >>>> The xfs and ocfs2 call generic_remap_file_range_prep to drop file >>>> privileges, I'm not sure whether btrfs should do the same thing. >>> >>> Why do you think btrfs shouldn't do the same thing. Looking at I'm not sure btrfs doesn't use generic check intentionally for some reason. >>> remap_file_range_prep it seems that btrfs is missing a ton of checks >>> that are useful i.e immutable files/aligned offsets etc. It is indeed. In addition, generic_remap_file_range_prep will invoke inode_dio_wait filemap_write_and_wait_range for the source and destination inode/range. For the dedupe case, it will call vfs_dedupe_file_range_compare. I still can't judge whether these operations are welcome by btrfs. I will go deep into the code. >> >> Any chance we could move btrfs over to use remap_file_range_prep so that >> all file systems share the exact same checks? In theory we can call generic_remap_file_range_prep in btrfs_remap_file_range, which give us the opportunity to clean up the duplicate check code in btrfs_extent_same and btrfs_clone_files. > >I'm not very familiar with the, Filipe is more familiar so adding to CC. >But IMO we should do that provided there are no blockers. > >Filipe, what do you think, is it feasible? I'm all ears for the suggestions.
On Wed, Nov 28, 2018 at 9:26 AM Lu Fengqi <lufq.fnst@cn.fujitsu.com> wrote: > > On Wed, Nov 28, 2018 at 09:48:07AM +0200, Nikolay Borisov wrote: > > > > > >On 28.11.18 г. 9:46 ч., Christoph Hellwig wrote: > >> On Wed, Nov 28, 2018 at 09:44:59AM +0200, Nikolay Borisov wrote: > >>> > >>> > >>> On 28.11.18 г. 5:07 ч., Lu Fengqi wrote: > >>>> The generic/513 tell that cloning into a file did not strip security > >>>> privileges (suid, capabilities) like a regular write would. > >>>> > >>>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > >>>> --- > >>>> The xfs and ocfs2 call generic_remap_file_range_prep to drop file > >>>> privileges, I'm not sure whether btrfs should do the same thing. > >>> > >>> Why do you think btrfs shouldn't do the same thing. Looking at > > I'm not sure btrfs doesn't use generic check intentionally for some reason. > > >>> remap_file_range_prep it seems that btrfs is missing a ton of checks > >>> that are useful i.e immutable files/aligned offsets etc. > > It is indeed. > > In addition, generic_remap_file_range_prep will invoke inode_dio_wait > filemap_write_and_wait_range for the source and destination inode/range. > For the dedupe case, it will call vfs_dedupe_file_range_compare. > > I still can't judge whether these operations are welcome by btrfs. I > will go deep into the code. > > >> > >> Any chance we could move btrfs over to use remap_file_range_prep so that > >> all file systems share the exact same checks? > > In theory we can call generic_remap_file_range_prep in > btrfs_remap_file_range, which give us the opportunity to clean up the > duplicate check code in btrfs_extent_same and btrfs_clone_files. > > > > >I'm not very familiar with the, Filipe is more familiar so adding to CC. > >But IMO we should do that provided there are no blockers. > > > >Filipe, what do you think, is it feasible? > > I'm all ears for the suggestions. There's no reason why it shouldn't be possible to have them called in btrfs as well. There's quite a few changes in vfs and generic functions introduced in 4.20 due to reflink/dedupe bugs, probably either at the time, or when cloning/dedupe stopped being btrfs specific, someone forgot to make btrfs use those generic vfs helpers. I'll take a look as well. > > -- > Thanks, > Lu > >
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 410c7e007ba8..bc33c480603b 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4312,6 +4312,10 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, goto out_unlock; } + ret = file_remove_privs(file); + if (ret) + goto out_unlock; + if (destoff > inode->i_size) { ret = btrfs_cont_expand(inode, inode->i_size, destoff); if (ret)
The generic/513 tell that cloning into a file did not strip security privileges (suid, capabilities) like a regular write would. Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> --- The xfs and ocfs2 call generic_remap_file_range_prep to drop file privileges, I'm not sure whether btrfs should do the same thing. Any suggestion? fs/btrfs/ioctl.c | 4 ++++ 1 file changed, 4 insertions(+)