[RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Lu Fengqi]

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(+)

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)
-- 
2.19.2

Re: [RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Nikolay Borisov]

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)
> 

Re: [RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Christoph Hellwig]

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?

Re: [RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Nikolay Borisov]

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?

> 

Re: [RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Lu Fengqi]

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.

-- 
Thanks,
Lu

Re: [RFC PATCH] btrfs: drop file privileges in btrfs_clone_files

[Filipe Manana]

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
>
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”