[RFC] btrfs: drop file privileges in btrfs_clone_files
diff mbox series

Message ID 20181128030731.10288-1-lufq.fnst@cn.fujitsu.com
State New
Headers show
Series
  • [RFC] btrfs: drop file privileges in btrfs_clone_files
Related show

Commit Message

Lu Fengqi Nov. 28, 2018, 3:07 a.m. UTC
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(+)

Comments

Nikolay Borisov Nov. 28, 2018, 7:44 a.m. UTC | #1
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)
>
Christoph Hellwig Nov. 28, 2018, 7:46 a.m. UTC | #2
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?
Nikolay Borisov Nov. 28, 2018, 7:48 a.m. UTC | #3
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?

>
Lu Fengqi Nov. 28, 2018, 9:24 a.m. UTC | #4
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.
Filipe Manana Nov. 28, 2018, 10:34 a.m. UTC | #5
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
>
>

Patch
diff mbox series

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)