[PATCH] btrfs: change btrfs_csum_final result param type to u8

[Domagoj Tršan]

---
 fs/btrfs/disk-io.c | 2 +-
 fs/btrfs/disk-io.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 50bed6c..95bd34f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -273,7 +273,7 @@ u32 btrfs_csum_data(char *data, u32 seed, size_t len)
return btrfs_crc32c(seed, data, len);
 }
 
-void btrfs_csum_final(u32 crc, char *result)
+void btrfs_csum_final(u32 crc, u8 *result)
 {
put_unaligned_le32(~crc, result);
 }
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 8e79d00..e849845 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -118,7 +118,7 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 
parent_transid,
  int atomic);
 int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid);
 u32 btrfs_csum_data(char *data, u32 seed, size_t len);
-void btrfs_csum_final(u32 crc, char *result);
+void btrfs_csum_final(u32 crc, u8 *result);
 int btrfs_bio_wq_end_io(struct btrfs_fs_info *info, struct bio *bio,
enum btrfs_wq_endio_type metadata);
 int btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct inode *inode,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] btrfs: change btrfs_csum_final result param type to u8

[Domagoj Tršan]

csum member of struct btrfs_super_block has array type of u8. It makes sense
that function btrfs_csum_final should be also declared to accept u8 *. I
changed the declaration of method void btrfs_csum_final(u32 crc, char *result);
to void btrfs_csum_final(u32 crc, u8 *result);

Domagoj Tršan (1):
  btrfs: change btrfs_csum_final result param type to u8

 fs/btrfs/disk-io.c | 2 +-
 fs/btrfs/disk-io.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] btrfs-progs: change btrfs_csum_final result param type to u8

[Domagoj Tršan]

Signed-off-by: Domagoj Tršan 
---
 btrfs-image.c |  2 +-
 chunk-recover.c   |  2 +-
 cmds-check.c  |  2 +-
 cmds-inspect-dump-super.c |  2 +-
 disk-io.c | 10 +-
 disk-io.h |  2 +-
 file-item.c   |  2 +-
 free-space-cache.c|  2 +-
 super-recover.c   |  2 +-
 utils.c   |  2 +-
 10 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 953d368..6de163c 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -163,7 +163,7 @@ static struct extent_buffer *alloc_dummy_eb(u64 bytenr, u32 
size);
 
 static void csum_block(u8 *buf, size_t len)
 {
-   char result[BTRFS_CRC32_SIZE];
+   u8 result[BTRFS_CRC32_SIZE];
u32 crc = ~(u32)0;
crc = crc32c(crc, buf + BTRFS_CSUM_SIZE, len - BTRFS_CSUM_SIZE);
btrfs_csum_final(crc, result);
diff --git a/chunk-recover.c b/chunk-recover.c
index 4a081db..32d2805 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1914,7 +1914,7 @@ static int check_one_csum(int fd, u64 start, u32 len, u32 
tree_csum)
}
ret = 0;
csum_result = btrfs_csum_data(NULL, data, csum_result, len);
-   btrfs_csum_final(csum_result, (char *)_result);
+   btrfs_csum_final(csum_result, (u8 *)_result);
if (csum_result != tree_csum)
ret = 1;
 out:
diff --git a/cmds-check.c b/cmds-check.c
index a453696..a36693d 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -5740,7 +5740,7 @@ again:
 
csum = btrfs_csum_data(NULL, (char *)data + tmp,
   csum, root->sectorsize);
-   btrfs_csum_final(csum, (char *));
+   btrfs_csum_final(csum, (u8 *));
 
csum_offset = leaf_offset +
 tmp / root->sectorsize * csum_size;
diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c
index aab5075..2f2c628 100644
--- a/cmds-inspect-dump-super.c
+++ b/cmds-inspect-dump-super.c
@@ -37,7 +37,7 @@
 
 static int check_csum_sblock(void *sb, int csum_size)
 {
-   char result[BTRFS_CSUM_SIZE];
+   u8 result[BTRFS_CSUM_SIZE];
u32 crc = ~(u32)0;
 
crc = btrfs_csum_data(NULL, (char *)sb + BTRFS_CSUM_SIZE,
diff --git a/disk-io.c b/disk-io.c
index f5340c3..c16eda2 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -123,7 +123,7 @@ u32 btrfs_csum_data(struct btrfs_root *root, char *data, 
u32 seed, size_t len)
return crc32c(seed, data, len);
 }
 
-void btrfs_csum_final(u32 crc, char *result)
+void btrfs_csum_final(u32 crc, u8 *result)
 {
put_unaligned_le32(~crc, result);
 }
@@ -131,7 +131,7 @@ void btrfs_csum_final(u32 crc, char *result)
 static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size,
  int verify, int silent)
 {
-   char result[BTRFS_CSUM_SIZE];
+   u8 result[BTRFS_CSUM_SIZE];
u32 len;
u32 crc = ~(u32)0;
 
@@ -1423,7 +1423,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char 
*path, u64 sb_bytenr,
  */
 static int check_super(struct btrfs_super_block *sb, unsigned sbflags)
 {
-   char result[BTRFS_CSUM_SIZE];
+   u8 result[BTRFS_CSUM_SIZE];
u32 crc;
u16 csum_type;
int csum_size;
@@ -1647,7 +1647,7 @@ static int write_dev_supers(struct btrfs_root *root,
crc = ~(u32)0;
crc = btrfs_csum_data(NULL, (char *)sb + BTRFS_CSUM_SIZE, crc,
  BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
-   btrfs_csum_final(crc, (char *)>csum[0]);
+   btrfs_csum_final(crc, >csum[0]);
 
/*
 * super_copy is BTRFS_SUPER_INFO_SIZE bytes and is
@@ -1671,7 +1671,7 @@ static int write_dev_supers(struct btrfs_root *root,
crc = ~(u32)0;
crc = btrfs_csum_data(NULL, (char *)sb + BTRFS_CSUM_SIZE, crc,
  BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
-   btrfs_csum_final(crc, (char *)>csum[0]);
+   btrfs_csum_final(crc, >csum[0]);
 
/*
 * super_copy is BTRFS_SUPER_INFO_SIZE bytes and is
diff --git a/disk-io.h b/disk-io.h
index c404d3f..1080fc1 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -175,7 +175,7 @@ int btrfs_set_buffer_uptodate(struct extent_buffer *buf);
 int wait_on_tree_block_writeback(struct btrfs_root *root,
 struct extent_buffer *buf);
 u32 btrfs_csum_data(struct btrfs_root *root, char *data, u32 seed, size_t len);
-void btrfs_csum_final(u32 crc, char *result);
+void btrfs_csum_final(u32 crc, u8 *result);
 
 int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 struct btrfs_root *root);
diff --git a/file-item.c b/file-item.c
index 7a3bbf3..dc3d404 100644
--- a/file-item.c
+++ b/file-item.c
@@ -297,7 

[PATCH] btrfs-progs: change btrfs_csum_final result param type to u8

[Domagoj Tršan]

csum member of struct btrfs_super_block has array type of u8. It makes sense
that function btrfs_csum_final should be also declared to accept u8 *. I
changed the declaration of method void btrfs_csum_final(u32 crc, char *result);
to void btrfs_csum_final(u32 crc, u8 *result);
Also, I changed definitions of various csum variables to be consistent with
kernel code. In kernel code they are defined as u8 result[BTRFS_CSUM_SIZE] but
here was as char result[BTRFS_CSUM_SIZE].

Domagoj Tršan (1):
  btrfs-progs: change btrfs_csum_final result param type to u8

 btrfs-image.c |  2 +-
 chunk-recover.c   |  2 +-
 cmds-check.c  |  2 +-
 cmds-inspect-dump-super.c |  2 +-
 disk-io.c | 10 +-
 disk-io.h |  2 +-
 file-item.c   |  2 +-
 free-space-cache.c|  2 +-
 super-recover.c   |  2 +-
 utils.c   |  2 +-
 10 files changed, 14 insertions(+), 14 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[David Sterba]

On Fri, Sep 16, 2016 at 07:56:02PM +0800, Anand Jain wrote:
> 
> 
> >> however here below is the quick example
> >> on the cli usage. Please try out, let me know if I have missed something.
> >>
> >> Also would like to mention that a review from the security experts is due,
> >> which is important and I believe those review comments can be accommodated
> >> without major changes from here.
> >
> > I disagree. Others commented on the crypto stuff, I see enough points to
> > address that would lead to major changes.
> >
> >> Also yes, thanks for the emails, I hear, per file encryption and inline
> >> with vfs layer is also important, which is wip among other things in the
> >> list.
> >
> > Implementing the recent vfs encryption in btrfs is ok, it's just feature
> > parity using an existing API.
> 
> 
>   As mentioned 'inline with vfs layer' I mean to say to use
>   fs/crypto KPIs. Which I haven't seen what parts of the code
>   from ext4 was made as generic KPIs. If that's getting stuff
>   correct in the encryption related, I think it would here as well.

So you were not talking about the 'fs/crypto' that was merged in 4.6?

>   Internal to btrfs - I had challenges to get the extents encoding
>   done properly without bailout, and the test plan. Which I think
>   is addressed here in this code. as mentioned.

Sorry, I don't understand what you mean.

> > And a note from me with maintainer's hat on, there are enough pending
> > patches and patchsets that need review, and bugs to fix, I'm not going
> > to spend time on something that we don't need at the moment if there are
> > alternatives.
> 
>   Honestly I agree. I even suggested but I had no choice.
> 
> 
> PS:
>   Pls, feel free to flame on the (raid) patches if its not correct,
>   because its rather more productive than no reply.

If it's the hot-spare and auto-replace feature, I've expressed my stance
in http://marc.info/?l=linux-btrfs=146252575330106, there hasn't
been any change. IMO the hot-spare feature makes most sense with the
raid56, which is stuck where it is, so we need to get it working first.

Lack of reply usually means lack of time (which I would not spend on
flaming but evaluation and review).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[Chris Murphy]

On Sat, Sep 17, 2016 at 10:12 AM, Anand Jain  wrote:

>   btrfs keeps it only in-memory and key hash goes to the disk.
>   Further in the long we need an integration with key management
>   system as well.

Maybe LUKS2 is usable for this part, and still adaptable since it's
not finished yet? It looks to me like essentially unlimited keyslots
compared to the current 8. You don't really care about the dm-crypt
part of it, but the key management part of it, perhaps.

Both the original and new subvolumes initially share one DEK that go
with the shared encrypted extents, but upon snapshot happening the new
extents in each subvolume need their own DEK. Policy wise these DEKs
can be wrapped in the same or separate passphrases or KEKs, as there
could be hundreds or thousands of DEKs that apply to the many possible
shared encrypted extents in a subvolume. If that's true, then it's an
explosive number of keys per subvolume potentially. It doesn't depend
on space as much as it depends on fs lifetime

Otherwise I don't see how this is different than using a single DEK
across all company hard drives. Compromise one, you've compromised
them all.



-- 
Chris Murphy
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[David Sterba]

On Sat, Sep 17, 2016 at 12:38:30AM -0400, Zygo Blaxell wrote:
> There's also a nasty problem with the extent tree--there's only one per
> filesystem, it's shared between all subvols and block groups, and every
> extent in that tree has back references to the (possibly encrypted) subvol
> trees.  I'll leave that problem as an exercise for other readers.  ;)

A design point that I'm not mentioning for the first time: there would
be per-subvolume group extent trees, ie. a set of subvolumes with
attached extent tree where similar to what we have now. So, encrypted
and unencrypted extent metadata will never be mixed.
(the crypto key questions are not addressed here)

This hasn't been implemented but I'm making sure this will be possible
when somebody mentions changes to the extent tree or blockgroup reworks
(to actually solve other problems).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[Anand Jain]

Hi Eric,

 Thanks for the constructive feedback, pls see inline below.


On 09/17/2016 02:58 PM, Eric Biggers wrote:

On Tue, Sep 13, 2016 at 09:39:46PM +0800, Anand Jain wrote:


This patchset adds btrfs encryption support.



Hi Anand,

I'm part of a team that will be maintaining and improving ext4 encryption.
Because f2fs now shares much of the code, it will benefit from the ext4
encryption work too.  It would be really nice if btrfs could be in the same
boat, since that would avoid redundant work and help prevent bugs and design
flaws.  So I strongly suggest that you explore how btrfs encryption might reuse
the existing code (and maybe extend it if there is very good reason to).


In fact my first attempt was using f2fs/ext4, found its too complicated,
further couldn't stable it, so re-wrote completely to a version where I
won't worry too much on the cipher mode _at the moment_, so this version
came with a caveat as mentioned.
Now looking to integrate with fs/crypto, however have the following 
concerns,


fs/crypto:
.
  fscrypt_context:master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
  should it go to the disk ? its just a descriptor which might
  change at the user end and still may contain the right key
  in the payload.

  btrfs keeps it only in-memory and key hash goes to the disk.
  Further in the long we need an integration with key management
  system as well.

.
  ext4/f2fs allows per file keys, when we are talking about large
  data center FS for cloud services, it would need key services
  to scale along with the FS. And there will be a lot of resources
  which is allocated but not used.

.
 system keyring already defines struct user_key_payload
 probably we should have used it instead of

 71 struct fscrypt_key {
 72 u32 mode;
 73 u8 raw[FS_MAX_KEY_SIZE];
 74 u32 size;
 75 } __packed;


.
 Some of key derivation functions should have been part of
 crypto library rather.

.
 page->index based IV won't suite btrfs, so it uses a random, but yes
 it needs crypto hardened. I am kind of opinion that, for a real need
 of retrievable random number we are replacing it with sector(truecrypt)
 /page-offset number, I think we aren't addressing problem in a right
 way ? If that that's the best solution we could achieve, yet I am not
 sure how to solve the need of FS independent decrypt ? I am yet to
 look at gpg.

.
 Yes needs AEAD. But at the same time we need
   - MAC to be separated from the ciphertext and ciphertext-size
 == plaintext-size.
   To make sure for sync,dio we do create extents and IO which matches
   with the application IO. So that performance tuning will be things
   as usual.



There also needs to be a proper design document for btrfs encryption.  This is
especially true if for some (very, very good) reason you can't reuse the
infrastructure from ext4 and f2fs.  There also could be unique challenges for
btrfs such as encryption keys and/or IVs being reused in reflinked extents.

You will also not get a proper review without a proper design document which
details things like the threat model and the security properties provided.  But
I did take a short look at the code anyway because I was interested.


 Thank You !!  I should have sent the code when doc is ready rather.
 Sorry about that.



 The
results were not pretty.  As far as I can see the current proposal is fatally
flawed as it does not provide confidentiality of file contents against a basic
attack.

The main two flaws are:

1. Use of CTR mode of operation
2. Reuse of same (key, IV) pair for all pages of a given inode

CTR mode is well known to fall over completely when used with repeated (key, IV)
pairs.  This makes the encryption nearly worthless.  In more detail: suppose I
am an attacker who has access to a file's ciphertext.  By the definition of CTR
mode, each ciphertext block is given by: C = E(ctr) XOR P, where C and P denote
the ciphertext and plaintext blocks respectively, E denotes encryption with the
block cipher using the secret key, and 'ctr' denotes the counter value.  Due to
flaw (2) the ctr values repeat every page.  Consequently, if I can correctly
guess the plaintext P1 of *any* page in the file and I want to know the
plaintext P2 of some other page, I can trivially compute P2 = P1 XOR C1 XOR C2.
No secret key needed.

Essentially: if there is any part of a file which is easily guessable, such as
a header or even a zeroed region, then the whole file is revealed.


 Yes this will be fixed. No TFM is claimed to be btrfs default
 as of now.


The solution is to use a less brittle mode of operation such as XTS in
combination with per-page IVs (or "tweaks") and derived per-file keys.  This is
already done in ext4 and f2fs, where the per-page IV is just the page offset.
Note that per-file keys were needed to prevent the same (key, IV) pair from
being used in multiple places.  So if you could reuse the fs/crypto
infrastructure, you could take advantage of the fact that this problem 

Re: Size of scrubbed Data

[Chris Murphy]

On Sat, Sep 17, 2016 at 8:34 AM, Tim Walberg  wrote:
> On 09/15/2016 15:18 -0600, Chris Murphy wrote:
>>>  > System, single: total=4.00MiB, used=0.00B
>>>  > Metadata, RAID1: total=10.00GiB, used=8.14GiB
>>>  > GlobalReserve, single: total=512.00MiB, used=0.00B
>>>
>>>  btrfs balance start -mconvert=raid1,soft 
>
>
> Since the single profile data is single, that should probably be
> "-s" instead of "-m"... I.e.
>
> btrfs balance start -sconvert=raid1,soft 
>
> It may also need "-f", since btrfs balance is a bit more paranoid
> about reprofiling system data.

Originally -m only handled metadata, not system chunks. But these days
-m includes metadata and system chunks, where -s is only system
chunks.

-- 
Chris Murphy
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Size of scrubbed Data

[Tim Walberg]

On 09/17/2016 09:34 -0500, Walberg, Tim wrote:
>>  On 09/15/2016 15:18 -0600, Chris Murphy wrote:
>>  >>  > System, single: total=4.00MiB, used=0.00B
>>  >>  > Metadata, RAID1: total=10.00GiB, used=8.14GiB
>>  >>  > GlobalReserve, single: total=512.00MiB, used=0.00B
>>  >>  
>>  >>  btrfs balance start -mconvert=raid1,soft 
>>  
>>  


Gaaa... I meant "since the single profile data is system, ..."

>>  Since the single profile data is single, that should probably be
>>  "-s" instead of "-m"... I.e.
>>  
>>  btrfs balance start -sconvert=raid1,soft 
>>  
>>  It may also need "-f", since btrfs balance is a bit more paranoid
>>  about reprofiling system data.
>>  
>>  
>>  
>>  -- 
>>  twalb...@gmail.com
>>  --
>>  To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 
>> in
>>  the body of a message to majord...@vger.kernel.org
>>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
End of included message



-- 
twalb...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Size of scrubbed Data

[Tim Walberg]

On 09/15/2016 15:18 -0600, Chris Murphy wrote:
>>  > System, single: total=4.00MiB, used=0.00B
>>  > Metadata, RAID1: total=10.00GiB, used=8.14GiB
>>  > GlobalReserve, single: total=512.00MiB, used=0.00B
>>  
>>  btrfs balance start -mconvert=raid1,soft 


Since the single profile data is single, that should probably be
"-s" instead of "-m"... I.e.

btrfs balance start -sconvert=raid1,soft 

It may also need "-f", since btrfs balance is a bit more paranoid
about reprofiling system data.



-- 
twalb...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Size of scrubbed Data

[Adam Borowski]

On Sat, Sep 17, 2016 at 01:02:18PM +0200, Stefan Malte Schumacher wrote:
> Concerning Testing: I only use it on desktops and home servers, which
> do not offer any services to the net. I am very fond of Debian and
> always use stable or even old-stable on work servers, but was finally
> to annoyed at the fact that the software I was using on a fresh
> installation was practically already out of date.

These days *-backports has pretty much everything you'd want for a real
reason, and maintainers tend to be forthcoming when asked; the rules allow
anyone with upload rights to backport anything (the string attached is
"you're then responsible for bugs and other problems") but it's customary to
ask the regular maintainer first.

Of course you won't get bling this way -- it's unreasonable to backport more
than a few dependant packages so a desktop environment is right out.  But
server packages, a major new version of an editor or mail client, or
sometimes even a new game are fine.

Obviously, stuff like new kernels or new btrfs-progs tend to come fast.

On the other hand, this means using testing outside of (soft-) freezes
became far less supported than it used to be in the past.

-- 
Second "wet cat laying down on a powered-on box-less SoC on the desk" close
shave in a week.  Protect your ARMs, folks!
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Size of scrubbed Data

[Stefan Malte Schumacher]

Hello

Thanks for the information. I did another run of scrubbing et voila:
total bytes scrubbed: 12.11TiB with 0 errors, which is pretty much two
times the amount of actual data stored on the filesystem. I am
relieved, but still would like to know why this went wrong in the
first place and why it is working again without me doing anything to
fix it.
Concerning Testing: I only use it on desktops and home servers, which
do not offer any services to the net. I am very fond of Debian and
always use stable or even old-stable on work servers, but was finally
to annoyed at the fact that the software I was using on a fresh
installation was practically already out of date.

Yours sincerely
Stefan

2016-09-16 14:21 GMT+02:00 Nicholas Steeves :
> Thank you for the assistance Chris :-)
>
> On 15 September 2016 at 17:18, Chris Murphy  wrote:
>>> On Thu, Sep 15, 2016 at 9:48 AM, Stefan Malte Schumacher
>>>  wrote:
> ...
>>> I believe it may be a result of replacing my old installation of
>>> Debian Jessie with Debian Stretch
> ...
>>
>>>
>>> btrfs --version
>>> btrfs-progs v4.7.1
>>
>> Upgrade to 4.7.2 or downgrade to 4.6.1 before using btrfs check; see
>> the changelog for details. I'm not recommending that you use btrfs
>> check, just saying this version of tools is not reliable for some
>> file systems.
>
> Hi Stefan, as far as I can tell 4.7.2 is currently blocked from
> migrating from unstable to testing due to a glibc version transition,
> so the easiest thing to do is to get fall back on 4.6.1 found here:
> http://snapshot.debian.org/package/btrfs-progs/4.6.1-1/
>
>
> Cheers,
> Nicholas
>
> P.S. You're brave to run testing before the soft-freeze!  Occasionally
> security fixes in sid can't propagate to testing, because a transition
> like this is in progress ( https://www.debian.org/security/faq#testing
> ).
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: compress=lzo safe to use?

[Kai Krakow]

Am Mon, 12 Sep 2016 04:36:07 + (UTC)
schrieb Duncan <1i5t5.dun...@cox.net>:

> Again, I once thought all this was just the stage at which btrfs was, 
> until I found out that it doesn't seem to happen if btrfs compression 
> isn't being used.  Something about the way it recovers from checksum 
> errors on compressed data differs from the way it recovers from
> checksum errors on uncompressed data, and there's a bug in the
> compressed data processing path.  But beyond that, I'm not a dev and
> it gets a bit fuzzy, which also explains why I've not gone code
> diving and submitted patches to try to fix it, myself.

I suspect that may very well come from the decompression routine which
crashes - and not from btrfs itself. So essentially, the decompression
needs to be fixed instead (which probably slows it down by factors).

Only when this is tested and fixed, one should look into why btrfs
fails when decompression fails.

-- 
Regards,
Kai

Replies to list-only preferred.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[Alex Elsayed]

On Fri, 16 Sep 2016 23:58:31 -0700, Eric Biggers wrote:

> On Tue, Sep 13, 2016 at 09:39:46PM +0800, Anand Jain wrote:
>>
>> This patchset adds btrfs encryption support.
>>
>>
> Hi Anand,



> Note: even better would be an authenticated encryption mode.  That isn't
> yet done by ext4 or f2fs --- I think because there wasn't a good place
> to store a per-page authentication tag.  It would be interesting to know
> whether this would be possible for btrfs.

IMO, this is already a flawed framing - in particular, if encrypting at 
the extent level, one _should not_ be encrypting (or authenticating) 
individual pages. The meaningful unit is the extent, and encrypting at 
page granularity puts you right back where dmcrypt is: dealing with fixed-
size space, and needing to find somewhere else to put the auth tag.

This is not a good place to be, and I strongly suspect it motivated 
choosing XTS in the first place - something I feel is an _error_ in the 
long run, and a dangerous one. (IMO, anything _but_ AEAD should be 
forbidden in FS-level encryption.)

In a nonce-misuse-resistent AEAD, there _is_ no auth tag: There's some 
amount of inherent ciphertext expansion, and the ciphertext _cannot be 
decrypted at all_ unless all of it is present. In essence, a built-in all-
or-nothing transform.

You could, potentially, chop off part of that and store it elsewhere, but 
now you're dealing with significant added complexity, for absolutely zero 
gain.

If you're _not_ using a nonce-misuse-resistant AEAD, it's even worse: 
keeping the tag out-of-band makes it far too easy to fail to verify it, 
or verify it only after decrypting the ciphertext to plaintext. Bluntly: 
that is an immediate security vulnerability.

tl;dr: Don't encrypt pages, encrypt extents. They grow a little for the 
auth tag, and that's fine.

Btrfs already handles needing to read the full extent in order to get a 
page out of it with compression, anyway.




--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[Eric Biggers]

On Tue, Sep 13, 2016 at 09:39:46PM +0800, Anand Jain wrote:
>
> This patchset adds btrfs encryption support.
>

Hi Anand,

I'm part of a team that will be maintaining and improving ext4 encryption.
Because f2fs now shares much of the code, it will benefit from the ext4
encryption work too.  It would be really nice if btrfs could be in the same
boat, since that would avoid redundant work and help prevent bugs and design
flaws.  So I strongly suggest that you explore how btrfs encryption might reuse
the existing code (and maybe extend it if there is very good reason to).

There also needs to be a proper design document for btrfs encryption.  This is
especially true if for some (very, very good) reason you can't reuse the
infrastructure from ext4 and f2fs.  There also could be unique challenges for
btrfs such as encryption keys and/or IVs being reused in reflinked extents.

You will also not get a proper review without a proper design document which
details things like the threat model and the security properties provided.  But
I did take a short look at the code anyway because I was interested.  The
results were not pretty.  As far as I can see the current proposal is fatally
flawed as it does not provide confidentiality of file contents against a basic
attack.

The main two flaws are:

1. Use of CTR mode of operation
2. Reuse of same (key, IV) pair for all pages of a given inode

CTR mode is well known to fall over completely when used with repeated (key, IV)
pairs.  This makes the encryption nearly worthless.  In more detail: suppose I
am an attacker who has access to a file's ciphertext.  By the definition of CTR
mode, each ciphertext block is given by: C = E(ctr) XOR P, where C and P denote
the ciphertext and plaintext blocks respectively, E denotes encryption with the
block cipher using the secret key, and 'ctr' denotes the counter value.  Due to
flaw (2) the ctr values repeat every page.  Consequently, if I can correctly
guess the plaintext P1 of *any* page in the file and I want to know the
plaintext P2 of some other page, I can trivially compute P2 = P1 XOR C1 XOR C2.
No secret key needed.

Essentially: if there is any part of a file which is easily guessable, such as
a header or even a zeroed region, then the whole file is revealed.

The solution is to use a less brittle mode of operation such as XTS in
combination with per-page IVs (or "tweaks") and derived per-file keys.  This is
already done in ext4 and f2fs, where the per-page IV is just the page offset.
Note that per-file keys were needed to prevent the same (key, IV) pair from
being used in multiple places.  So if you could reuse the fs/crypto
infrastructure, you could take advantage of the fact that this problem was
already solved.

Note: even better would be an authenticated encryption mode.  That isn't yet
done by ext4 or f2fs --- I think because there wasn't a good place to store a
per-page authentication tag.  It would be interesting to know whether this would
be possible for btrfs.

I also noticed that unlike ext4 and f2fs, filenames and symlinks are not being
encrypted in btrfs.  I know it may seem somewhat ad-hoc that filenames are
encrypted but not other metadata, but apparently filenames were considered
quite important and a lot of work went into making it possible to encrypt them
in ext4.

(Apologies if I misunderstood anything.  The proposal would be easier to review
with a design document, as mentioned.)

Hope this is helpful,

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Preliminary BTRFS Encryption

[Alex Elsayed]

On Sat, 17 Sep 2016 00:38:30 -0400, Zygo Blaxell wrote:

> On Fri, Sep 16, 2016 at 06:49:53AM +, Alex Elsayed wrote:
>> The main issue I see is that subvolumes as btrfs has them _do_
>> introduce novel concerns - in particular, how should snapshots interact
>> with keying (and nonces)? None of the AEADs currently in the kernel are
>> nonce-misuse resistant, which means that if different data is encrypted
>> under the same key and nonce, things go _very_ badly wrong. With
>> writable snapshots, I'd consider that a nontrivial risk.
> 
> Snapshots should copy subvolume keys (or key UUIDs, since the keys
> aren't stored in the filesystem), i.e. an ioctl could say "create a new
> subvol 'foo' with the same key as existing subvol 'bar'".  This could
> also handle nested subvols (child copies key of parent) if the nested
> subvols weren't created with their own separate keys.  For snapshots,
> we wouldn't even ask--the snapshot and its origin subvol would share a
> key unconditionally. (*)

I'll quote the LWN article on the way EXT4 (and VFS) encryption works 
(https://lwn.net/Articles/639427/):

> Encryption in ext4 is a per-directory-tree affair. One starts by
> setting an encryption policy (using an ioctl() call) for a given
> directory, which must be empty at the time; that policy includes a
> master key used for all files and directories stored below the target
> directory. Each individual file is encrypted with its own key, which is
> derived from the master key and a per-file random nonce value (which is
> stored in an extended attribute attached to the file's inode). File
> names and symbolic links are also encrypted.

So there isn't quite a "subvol key" in the VFS approach - each directory 
has a key, and there are derived keys for the entries below it. (I'll 
note that this framing does not address shared extents _at all_, and 
would love to have clarification on that).

> I don't see how snapshots could work, writable or otherwise, without
> separating the key identity from the subvol identity and having a
> many-to-one relationship between subvols and keys.  The extents in each
> subvol would be shared, and they'd be encrypted with a single secret,
> so there's not really another way to do this.

That's not the issue. The issue is that, assuming the key stays the same, 
then a user could quite possibly create a snapshot, write into both the 
original and the snapshot, causing encryption to occur twice with the 
same key, same nonce, and different data.

This invalidates both the integrity and confidentialiyy of AES-GCM (and 
any other AEAD that is not nonce-misuse resistant), allowing them to 
effectively mount offline decryption attacks against things they could 
not ordinarily read, or replace files without being caught.

> If the key is immutable (which it probably is, given that it's used to
> encrypt at the extent level, and extents are (mostly) immutable) then
> just giving each subvol a copy of the key ID is sufficient.

Sufficient for reading data, yes. Sufficient for really nasty nonce-reuse 
attacks, also yes.

> (*) OK, we could ask, but if the answer was "no, please do not use the
> origin subvol's key", then btrfs would return EINVAL and not create the
> snapshot, since there would be no way to read any data contained within
> it without the key.

It _might_ be possible to get away with only allowing RO snapshots for 
encrypted subvols, but this really requires much more careful thought 
than it's getting.

>> > Indeed, with the generic file encryption, btrfs may not even need the
>> > special subvolume encryption pixies. i.e. you can effectively
>> > implement subvolume encryption via configuration of a multi-user
>> > encryption key for each subvolume and apply it to the subvolume tree
>> > root at creation time. Then only users with permission to unlock the
>> > subvolume key can access it.
> 
> Life is pretty easy when we're only encrypting data extents.

Agreed.

> Encrypted subvol trees cause quite a few problems for btrfs when it
> needs to relocate extents (e.g. to shrink a filesystem or change RAID
> profile) or validate data integrity.  Ideally it would still be able to
> do these operations without decrypting the data; otherwise, there are
> bad cases, e.g. if a disk fails, all of the subvolumes would have to be
> unlocked in order to replace a disk.

Sure; there are certainly caveats. At very least, the free space map 
would need to be either unencrypted, or encrypted under a "global" key 
(no worse in security properties than dmcrypt, though, with the added 
boon of AEAD). That would at least make operation with some subvolumes 
locked safe.

One could also just encrypt all of the subvolume trees under one global 
key. Encrypting _all_ the metadata in _some fashion_ is _required_ if we 
ever want dmcrypt to be unnecessary, but if that's the goal, per-
subvolume keys for that aren't needed (though they would be nice).

(If making dmcrypt unnecessary is a non-goal, then