On 2015-02-24 04:32:21, Rocko wrote:
> I think it is still useful for ecryptfs to support the btrfs clone ioctl
> for the case where both source and target higher files are in the same
> ecryptfs mount, since this saves disk space.

I don't like the idea of eCryptfs supporting the clone ioctl by default.
It would allow an attacker to discover that the files (the original and
the clone) are the same. Sure, the decrypted file size of eCryptfs files
is stored in plaintext so an attacker would already know that the two
files are the same size but that doesn't mean that they contain the same
contents.

A core design aspect of eCryptfs is that a unique File Encryption Key
(FEK) is generated for each file, meaning that two matching plaintext
files will have entirely different ciphertext.

I'd consider allowing support for the clone ioctl down the road if the
user specified an eCryptfs mount option that implies that
performance and efficiency are more important than security.

Supporting sparse files in eCryptfs could fall under that same mount
option.

> We might be able to handle this in
> fs/ecryptfs/file.c#ecryptfs_unlocked_ioctl, which gets passed the btrfs
> ioctrl clone command along with the the higher target file. It
> unconditionally converts it to the lower file and then passes it down to
> btrfs:
> 
>   struct file *lower_file = ecryptfs_file_to_lower(file);
>    ...
>   if (lower_file->f_op->unlocked_ioctl)
>     rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
> 
> Adding code here that returns failure if the command is BTRFS_IOC_CLONE
> makes the cp --reflink=always command fail. (cp --reflink=auto then
> works because cp detects the failure and does a non-clone copy, and the
> same should go for the glib file copy).
> 
> However, note that:
> 
> 1. When it fails to clone, ecryptfs still creates a valid but zero-byte
> file after the failed clone operation, so ecryptfs should probably
> remove this when it returns fail for the clone operation.

This is a bug in the cp program, if anything. The same thing happens on
ext4. Here's what we see from strace:

$ strace cp --reflink=always test test.copy
...
open("test", O_RDONLY)                  = 3
fstat(3, {st_mode=S_IFREG|0664, st_size=5, ...}) = 0
open("test.copy", O_WRONLY|O_CREAT|O_EXCL, 0664) = 4
fstat(4, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
ioctl(4, BTRFS_IOC_CLONE, 0x3)          = -1 ENOTTY (Inappropriate ioctl for device)
...

cp opens test, then creates test.copy, before calling ioctl() on
test.copy's file descriptor. The clone fails but then cp never unlinks
it.

> 2. It shouldn't fail if the source and target files are both inside the
> same ecryptfs mount.

I feel like it should fail for the reasons mentioned above.

If someone wants to add BTRFS clone support down the road, that'd be
nice. However, I think this bug is properly fixed by returning an error
when ecryptfs_{compat,unlocked}_ioctl() sees BTRFS_IOC_CLONE. That's the
only backportable fix.

In fact, I think eCryptfs should probably be returning error on any
filesystem specific ioctl commands. There are a handful ioctl commands
common across all filesystems that it should support and pass down to
the lower filesystem but all others are unknowns and should be
blacklisted.

> 3. Do symlinks affect it, eg if the target is a symlink outside the
> ecryptfs mount that points to the higher ecryptfs file?

Good question. That'll be something to test if anyone ever adds clone
support.

> 4. ecryptfs_compat_ioctl might possibly be affected as well since it
> does the same thing as ecryptfs_unlocked_ioctl.

I think you're right.