linux-kernel - Re: [PATCH v2] vfs: prevent copy_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <13583117-59D4-4294-BB23-9D4802E4B8A3@dilger.ca>
Date:   Tue, 16 Feb 2021 11:54:07 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Luis Henriques <lhenriques@...e.de>,
        Trond Myklebust <trondmy@...merspace.com>,
        "samba-technical@...ts.samba.org" <samba-technical@...ts.samba.org>,
        "drinkcat@...omium.org" <drinkcat@...omium.org>,
        "iant@...gle.com" <iant@...gle.com>,
        "linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
        "darrick.wong@...cle.com" <darrick.wong@...cle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jlayton@...nel.org" <jlayton@...nel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "llozano@...omium.org" <llozano@...omium.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "miklos@...redi.hu" <miklos@...redi.hu>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "dchinner@...hat.com" <dchinner@...hat.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "sfrench@...ba.org" <sfrench@...ba.org>,
        James Simmons <jsimmons@...radead.org>,
        "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>
Subject: Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices

On Feb 16, 2021, at 6:51 AM, Amir Goldstein <amir73il@...il.com> wrote:
>> 
>>> This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
>>> is internal to kernel users.
>>> 
>>> FWIW, you may want to look at the loop in ovl_copy_up_data()
>>> for improvements to nfsd_copy_file_range().
>>> 
>>> We can move the check out to copy_file_range syscall:
>>> 
>>>        if (flags != 0)
>>>                return -EINVAL;
>>> 
>>> Leave the fallback from all filesystems and check for the
>>> COPY_FILE_SPLICE flag inside generic_copy_file_range().
>> 
>> Ok, the diff bellow is just to make sure I understood your suggestion.
>> 
>> The patch will also need to:
>> 
>> - change nfs and overlayfs calls to vfs_copy_file_range() so that they
>>   use the new flag.
>> 
>> - check flags in generic_copy_file_checks() to make sure only valid flags
>>   are used (COPY_FILE_SPLICE at the moment).
>> 
>> Also, where should this flag be defined?  include/uapi/linux/fs.h?
>> 
>> Cheers,
>> --
>> Luis
>> 
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 75f764b43418..341d315d2a96 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -1383,6 +1383,13 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
>>                                struct file *file_out, loff_t pos_out,
>>                                size_t len, unsigned int flags)
>> {
>> +       if (!(flags & COPY_FILE_SPLICE)) {
>> +               if (!file_out->f_op->copy_file_range)
>> +                       return -EOPNOTSUPP;
>> +               else if (file_out->f_op->copy_file_range !=
>> +                        file_in->f_op->copy_file_range)
>> +                       return -EXDEV;
>> +       }
> 
> That looks strange, because you are duplicating the logic in
> do_copy_file_range(). Maybe better:
> 
> if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
>        return -EINVAL;
> if (flags & COPY_FILE_SPLICE)
>       return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
>                                 len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
> if (!file_out->f_op->copy_file_range)
>        return -EOPNOTSUPP;
> return -EXDEV;

This shouldn't return -EINVAL to userspace if the flag is not set.

That implies there *is* some valid way for userspace to call this
function, which is AFAICS not possible if COPY_FILE_SPLICE is only
available to in-kernel callers.  Instead, it should continue
to return -EOPNOTSUPP to userspace if copy_file_range() is not valid
for this combination of file descriptors, so that applications will
fall back to the non-CFR implementation.

The WARN_ON_ONCE(ret == -EOPNOTSUPP) in vfs_copy_file_range() would
also need to be removed if this will be triggered from userspace.


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)