(no subject)

[Mae Kasza]

This is my first time using git send-email,
hopefully everything's fine :)

[PATCH] cmd_set_passphrase: initialize KDF parameters

[Mae Kasza]

From: mae <git@badat.dev>

The set-passphrase command failed to derive the key for disks initially
formatted with --encrypted and --no_passphrase.

This happened because bch_sb_crypt_init only configures the KDF params
if a passphrase is specified.
This commit makes the command initialize the KDF with the same parameters
as bch_sb_crypt_init if the key wasn't encrypted before.

Signed-off-by: Mae Kasza <git@badat.dev>
---
 c_src/cmd_key.c |  9 +++++++--
 c_src/crypto.c  | 13 ++++++++-----
 c_src/crypto.h  |  1 +
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
index adb0ac8d..2da83758 100644
--- a/c_src/cmd_key.c
+++ b/c_src/cmd_key.c
@@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
 	if (IS_ERR(c))
 		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
 
-	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
+	struct bch_sb *sb = c->disk_sb.sb;
+	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
 	if (!crypt)
 		die("Filesystem does not have encryption enabled");
 
@@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
 		die("Error getting current key");
 
 	char *new_passphrase = read_passphrase_twice("Enter new passphrase: ");
+	if (!bch2_key_is_encrypted(&crypt->key)) {
+		bch_crypt_default_kdf_init(crypt);
+	}
+
 	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
 
-	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(c->disk_sb.sb),
+	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
 				    &new_key, sizeof(new_key)))
 		die("error encrypting key");
 	crypt->key = new_key;
diff --git a/c_src/crypto.c b/c_src/crypto.c
index 32671bd8..30ad92d4 100644
--- a/c_src/crypto.c
+++ b/c_src/crypto.c
@@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
 	get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
 
 	if (passphrase) {
-
-		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
-		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
-		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
-		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
+		bch_crypt_default_kdf_init(crypt);
 
 		struct bch_key passphrase_key = derive_passphrase(crypt, passphrase);
 
@@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
 		memzero_explicit(&passphrase_key, sizeof(passphrase_key));
 	}
 }
+
+void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
+		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
+		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
+		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
+		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
+}
diff --git a/c_src/crypto.h b/c_src/crypto.h
index baea6d86..846a8931 100644
--- a/c_src/crypto.h
+++ b/c_src/crypto.h
@@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const char *,
 void bch2_add_key(struct bch_sb *, const char *, const char *, const char *);
 void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
 		       const char *);
+void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
 
 #endif /* _CRYPTO_H */
-- 
2.45.2

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Kent Overstreet]

On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
> From: mae <git@badat.dev>
> 
> The set-passphrase command failed to derive the key for disks initially
> formatted with --encrypted and --no_passphrase.
> 
> This happened because bch_sb_crypt_init only configures the KDF params
> if a passphrase is specified.
> This commit makes the command initialize the KDF with the same parameters
> as bch_sb_crypt_init if the key wasn't encrypted before.
> 
> Signed-off-by: Mae Kasza <git@badat.dev>

This looks good to me, but I'm in the middle of a 20 hour drive and not
feeling super confident to review crypto stuff tonight, so maybe someone
else can go over it too before I pull it in..

> ---
>  c_src/cmd_key.c |  9 +++++++--
>  c_src/crypto.c  | 13 ++++++++-----
>  c_src/crypto.h  |  1 +
>  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
> index adb0ac8d..2da83758 100644
> --- a/c_src/cmd_key.c
> +++ b/c_src/cmd_key.c
> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>  	if (IS_ERR(c))
>  		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
>  
> -	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
> +	struct bch_sb *sb = c->disk_sb.sb;
> +	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>  	if (!crypt)
>  		die("Filesystem does not have encryption enabled");
>  
> @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>  		die("Error getting current key");
>  
>  	char *new_passphrase = read_passphrase_twice("Enter new passphrase: ");
> +	if (!bch2_key_is_encrypted(&crypt->key)) {
> +		bch_crypt_default_kdf_init(crypt);
> +	}
> +
>  	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
>  
> -	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(c->disk_sb.sb),
> +	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
>  				    &new_key, sizeof(new_key)))
>  		die("error encrypting key");
>  	crypt->key = new_key;
> diff --git a/c_src/crypto.c b/c_src/crypto.c
> index 32671bd8..30ad92d4 100644
> --- a/c_src/crypto.c
> +++ b/c_src/crypto.c
> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>  	get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>  
>  	if (passphrase) {
> -
> -		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
> -		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
> -		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
> -		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
> +		bch_crypt_default_kdf_init(crypt);
>  
>  		struct bch_key passphrase_key = derive_passphrase(crypt, passphrase);
>  
> @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>  		memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>  	}
>  }
> +
> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
> +		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
> +		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
> +		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
> +		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
> +}
> diff --git a/c_src/crypto.h b/c_src/crypto.h
> index baea6d86..846a8931 100644
> --- a/c_src/crypto.h
> +++ b/c_src/crypto.h
> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const char *,
>  void bch2_add_key(struct bch_sb *, const char *, const char *, const char *);
>  void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
>  		       const char *);
> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>  
>  #endif /* _CRYPTO_H */
> -- 
> 2.45.2
> 

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Hongbo Li]

On 2024/8/12 9:41, Kent Overstreet wrote:
> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>> From: mae <git@badat.dev>
>>
>> The set-passphrase command failed to derive the key for disks initially
>> formatted with --encrypted and --no_passphrase.
>>
>> This happened because bch_sb_crypt_init only configures the KDF params
>> if a passphrase is specified.
>> This commit makes the command initialize the KDF with the same parameters
>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>
>> Signed-off-by: Mae Kasza <git@badat.dev>
> 
> This looks good to me, but I'm in the middle of a 20 hour drive and not
> feeling super confident to review crypto stuff tonight, so maybe someone
> else can go over it too before I pull it in..
> 
>> ---
>>   c_src/cmd_key.c |  9 +++++++--
>>   c_src/crypto.c  | 13 ++++++++-----
>>   c_src/crypto.h  |  1 +
>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>> index adb0ac8d..2da83758 100644
>> --- a/c_src/cmd_key.c
>> +++ b/c_src/cmd_key.c
>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>   	if (IS_ERR(c))
>>   		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
>>   
>> -	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
>> +	struct bch_sb *sb = c->disk_sb.sb;
>> +	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>   	if (!crypt)
>>   		die("Filesystem does not have encryption enabled");
>>   
>> @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>>   		die("Error getting current key");
>>   
>>   	char *new_passphrase = read_passphrase_twice("Enter new passphrase: ");
>> +	if (!bch2_key_is_encrypted(&crypt->key)) {
>> +		bch_crypt_default_kdf_init(crypt);
It works, but using bch_sb_crypt_init to reinitialize crypt may be 
better. In bch_sb_crypt_init, it has initialized the crypt and also 
cleaned new_passphrase.

Thanks
Hongbo

>> +	}
>> +
>>   	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
>>   
>> -	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(c->disk_sb.sb),
>> +	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
>>   				    &new_key, sizeof(new_key)))
>>   		die("error encrypting key");
>>   	crypt->key = new_key;
>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>> index 32671bd8..30ad92d4 100644
>> --- a/c_src/crypto.c
>> +++ b/c_src/crypto.c
>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>   	get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>   
>>   	if (passphrase) {
>> -
>> -		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>> -		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>> -		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>> -		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>> +		bch_crypt_default_kdf_init(crypt);
>>   
>>   		struct bch_key passphrase_key = derive_passphrase(crypt, passphrase);
>>   
>> @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>   		memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>   	}
>>   }
>> +
>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>> +		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>> +		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>> +		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>> +		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>> +}
>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>> index baea6d86..846a8931 100644
>> --- a/c_src/crypto.h
>> +++ b/c_src/crypto.h
>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const char *,
>>   void bch2_add_key(struct bch_sb *, const char *, const char *, const char *);
>>   void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
>>   		       const char *);
>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>   
>>   #endif /* _CRYPTO_H */
>> -- 
>> 2.45.2
>>
> 
> 

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Mae Kasza]

On 8/12/24 04:50, Hongbo Li wrote:
>
>
> On 2024/8/12 9:41, Kent Overstreet wrote:
>> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>>> From: mae <git@badat.dev>
>>>
>>> The set-passphrase command failed to derive the key for disks initially
>>> formatted with --encrypted and --no_passphrase.
>>>
>>> This happened because bch_sb_crypt_init only configures the KDF params
>>> if a passphrase is specified.
>>> This commit makes the command initialize the KDF with the same 
>>> parameters
>>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>>
>>> Signed-off-by: Mae Kasza <git@badat.dev>
>>
>> This looks good to me, but I'm in the middle of a 20 hour drive and not
>> feeling super confident to review crypto stuff tonight, so maybe someone
>> else can go over it too before I pull it in..
>>
>>> ---
>>>   c_src/cmd_key.c |  9 +++++++--
>>>   c_src/crypto.c  | 13 ++++++++-----
>>>   c_src/crypto.h  |  1 +
>>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>>> index adb0ac8d..2da83758 100644
>>> --- a/c_src/cmd_key.c
>>> +++ b/c_src/cmd_key.c
>>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>       if (IS_ERR(c))
>>>           die("Error opening %s: %s", argv[1], 
>>> bch2_err_str(PTR_ERR(c)));
>>>   -    struct bch_sb_field_crypt *crypt = 
>>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>>> +    struct bch_sb *sb = c->disk_sb.sb;
>>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>>       if (!crypt)
>>>           die("Filesystem does not have encryption enabled");
>>>   @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>           die("Error getting current key");
>>>         char *new_passphrase = read_passphrase_twice("Enter new 
>>> passphrase: ");
>>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>> +        bch_crypt_default_kdf_init(crypt);
> It works, but using bch_sb_crypt_init to reinitialize crypt may be 
> better. In bch_sb_crypt_init, it has initialized the crypt and also 
> cleaned new_passphrase.
>
> Thanks
> Hongbo
>
bch_sb_crypt_init also reinitializes the crypt->key, so I can't just 
reuse it here.

I could extract it into a bch_crypt_update_passphrase function. 
Originally I wanted to avoid doing that to limit the scope of this 
patchset, but I'll give it a shot

Thanks for the review!

Mae

>>> +    }
>>> +
>>>       struct bch_key passphrase_key = derive_passphrase(crypt, 
>>> new_passphrase);
>>>   -    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>> __bch2_sb_key_nonce(c->disk_sb.sb),
>>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>> __bch2_sb_key_nonce(sb),
>>>                       &new_key, sizeof(new_key)))
>>>           die("error encrypting key");
>>>       crypt->key = new_key;
>>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>>> index 32671bd8..30ad92d4 100644
>>> --- a/c_src/crypto.c
>>> +++ b/c_src/crypto.c
>>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>       get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>>         if (passphrase) {
>>> -
>>> -        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>> -        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>> -        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>> -        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>> +        bch_crypt_default_kdf_init(crypt);
>>>             struct bch_key passphrase_key = derive_passphrase(crypt, 
>>> passphrase);
>>>   @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>           memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>>       }
>>>   }
>>> +
>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>>> +        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>> +        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>> +        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>> +        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>> +}
>>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>>> index baea6d86..846a8931 100644
>>> --- a/c_src/crypto.h
>>> +++ b/c_src/crypto.h
>>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const 
>>> char *,
>>>   void bch2_add_key(struct bch_sb *, const char *, const char *, 
>>> const char *);
>>>   void bch_sb_crypt_init(struct bch_sb *sb, struct 
>>> bch_sb_field_crypt *,
>>>                  const char *);
>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>>     #endif /* _CRYPTO_H */
>>> -- 
>>> 2.45.2
>>>
>>
>>
>

[PATCH] Extract bch_crypt_update_passphrase function

[Mae Kasza]

This also fixes a bug where the set-passphrase
command failed to derive the key for disks initially
formatted with --encrypted and --no_passphrase, due to
bch_sb_crypt_init not configuring the KDF params if
the passphrase wasn't specified during formatting.

Signed-off-by: Mae Kasza <git@badat.dev>
---
 c_src/cmd_key.c | 26 ++++++++++----------------
 c_src/crypto.c  | 43 ++++++++++++++++++++++++++++++++-----------
 c_src/crypto.h  |  3 +++
 3 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
index adb0ac8d..ac8a94a8 100644
--- a/c_src/cmd_key.c
+++ b/c_src/cmd_key.c
@@ -104,24 +104,19 @@ int cmd_set_passphrase(int argc, char *argv[])
 	if (IS_ERR(c))
 		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
 
-	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
+	struct bch_sb *sb = c->disk_sb.sb;
+	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
 	if (!crypt)
 		die("Filesystem does not have encryption enabled");
 
-	struct bch_encrypted_key new_key;
-	new_key.magic = BCH_KEY_MAGIC;
-
-	int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
+	struct bch_key key;
+	int ret = bch2_decrypt_sb_key(c, crypt, &key);
 	if (ret)
 		die("Error getting current key");
 
 	char *new_passphrase = read_passphrase_twice("Enter new passphrase: ");
-	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
 
-	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(c->disk_sb.sb),
-				    &new_key, sizeof(new_key)))
-		die("error encrypting key");
-	crypt->key = new_key;
+	bch_crypt_update_passphrase(sb, crypt, &key, new_passphrase);
 
 	bch2_revoke_key(c->disk_sb.sb);
 	bch2_write_super(c);
@@ -142,18 +137,17 @@ int cmd_remove_passphrase(int argc, char *argv[])
 	if (IS_ERR(c))
 		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
 
-	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
+	struct bch_sb *sb = c->disk_sb.sb;
+	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
 	if (!crypt)
 		die("Filesystem does not have encryption enabled");
 
-	struct bch_encrypted_key new_key;
-	new_key.magic = BCH_KEY_MAGIC;
-
-	int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
+	struct bch_key key;
+	int ret = bch2_decrypt_sb_key(c, crypt, &key);
 	if (ret)
 		die("Error getting current key");
 
-	crypt->key = new_key;
+	bch_crypt_update_passphrase(sb, crypt, &key, NULL);
 
 	bch2_write_super(c);
 	bch2_fs_stop(c);
diff --git a/c_src/crypto.c b/c_src/crypto.c
index 32671bd8..301dbebe 100644
--- a/c_src/crypto.c
+++ b/c_src/crypto.c
@@ -176,26 +176,47 @@ void bch_sb_crypt_init(struct bch_sb *sb,
 		       struct bch_sb_field_crypt *crypt,
 		       const char *passphrase)
 {
+	struct bch_key key;
+	get_random_bytes(&key, sizeof(key));
+
 	crypt->key.magic = BCH_KEY_MAGIC;
-	get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
+	crypt->key.key = key;
 
-	if (passphrase) {
+	bch_crypt_update_passphrase(sb, crypt, &key, passphrase);
+}
 
+void bch_crypt_update_passphrase(
+			struct bch_sb *sb,
+			struct bch_sb_field_crypt *crypt,
+			struct bch_key *key,
+			const char *new_passphrase)
+{
+
+	struct bch_encrypted_key new_key;
+	new_key.magic = BCH_KEY_MAGIC;
+	new_key.key = *key;
+
+	if(!new_passphrase) {
+		crypt->key = new_key;
+		return;
+	}
+
+	// If crypt already has an encrypted key reuse it's encryption params
+	if (!bch2_key_is_encrypted(&crypt->key)) {
 		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
 		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
 		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
 		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
+	}
 
-		struct bch_key passphrase_key = derive_passphrase(crypt, passphrase);
-
-		assert(!bch2_key_is_encrypted(&crypt->key));
+	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
 
-		if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
-					   &crypt->key, sizeof(crypt->key)))
-			die("error encrypting key");
+	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
+				    &new_key, sizeof(new_key)))
+		die("error encrypting key");
 
-		assert(bch2_key_is_encrypted(&crypt->key));
+	memzero_explicit(&passphrase_key, sizeof(passphrase_key));
 
-		memzero_explicit(&passphrase_key, sizeof(passphrase_key));
-	}
+	crypt->key = new_key;
+	assert(bch2_key_is_encrypted(&crypt->key));
 }
diff --git a/c_src/crypto.h b/c_src/crypto.h
index baea6d86..cd3baac8 100644
--- a/c_src/crypto.h
+++ b/c_src/crypto.h
@@ -19,4 +19,7 @@ void bch2_add_key(struct bch_sb *, const char *, const char *, const char *);
 void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
 		       const char *);
 
+void bch_crypt_update_passphrase(struct bch_sb *sb, struct bch_sb_field_crypt *crypt,
+			struct bch_key *key, const char *new_passphrase);
+
 #endif /* _CRYPTO_H */
-- 
2.45.2

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Hongbo Li]

On 2024/8/13 21:16, Mae Kasza wrote:
> 
> On 8/12/24 04:50, Hongbo Li wrote:
>>
>>
>> On 2024/8/12 9:41, Kent Overstreet wrote:
>>> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>>>> From: mae <git@badat.dev>
>>>>
>>>> The set-passphrase command failed to derive the key for disks initially
>>>> formatted with --encrypted and --no_passphrase.
>>>>
>>>> This happened because bch_sb_crypt_init only configures the KDF params
>>>> if a passphrase is specified.
>>>> This commit makes the command initialize the KDF with the same 
>>>> parameters
>>>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>>>
>>>> Signed-off-by: Mae Kasza <git@badat.dev>
>>>
>>> This looks good to me, but I'm in the middle of a 20 hour drive and not
>>> feeling super confident to review crypto stuff tonight, so maybe someone
>>> else can go over it too before I pull it in..
>>>
>>>> ---
>>>>   c_src/cmd_key.c |  9 +++++++--
>>>>   c_src/crypto.c  | 13 ++++++++-----
>>>>   c_src/crypto.h  |  1 +
>>>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>>>> index adb0ac8d..2da83758 100644
>>>> --- a/c_src/cmd_key.c
>>>> +++ b/c_src/cmd_key.c
>>>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>       if (IS_ERR(c))
>>>>           die("Error opening %s: %s", argv[1], 
>>>> bch2_err_str(PTR_ERR(c)));
>>>>   -    struct bch_sb_field_crypt *crypt = 
>>>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>>>> +    struct bch_sb *sb = c->disk_sb.sb;
>>>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>>>       if (!crypt)
>>>>           die("Filesystem does not have encryption enabled");
>>>>   @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>           die("Error getting current key");
>>>>         char *new_passphrase = read_passphrase_twice("Enter new 
>>>> passphrase: ");
>>>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>>> +        bch_crypt_default_kdf_init(crypt);
>> It works, but using bch_sb_crypt_init to reinitialize crypt may be 
>> better. In bch_sb_crypt_init, it has initialized the crypt and also 
>> cleaned new_passphrase.
>>
>> Thanks
>> Hongbo
>>
> bch_sb_crypt_init also reinitializes the crypt->key, so I can't just 
> reuse it here.
> 
Yeah, it will change crypt->key. It seems the main point is not clean 
the password.
Do we need keep the crypt->key even the password is changed? Or does the 
crypt->key like a random seed and need to be kept in the whole 
lifecycle? Sorry I am a bit confused about this.

Thanks,
Hongbo

> I could extract it into a bch_crypt_update_passphrase function. 
> Originally I wanted to avoid doing that to limit the scope of this 
> patchset, but I'll give it a shot
> 
> Thanks for the review!
> 
> Mae
> 
>>>> +    }
>>>> +
>>>>       struct bch_key passphrase_key = derive_passphrase(crypt, 
>>>> new_passphrase);
>>>>   -    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>> __bch2_sb_key_nonce(c->disk_sb.sb),
>>>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>> __bch2_sb_key_nonce(sb),
>>>>                       &new_key, sizeof(new_key)))
>>>>           die("error encrypting key");
>>>>       crypt->key = new_key;
>>>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>>>> index 32671bd8..30ad92d4 100644
>>>> --- a/c_src/crypto.c
>>>> +++ b/c_src/crypto.c
>>>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>       get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>>>         if (passphrase) {
>>>> -
>>>> -        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>> -        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>> -        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>> -        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>> +        bch_crypt_default_kdf_init(crypt);
>>>>             struct bch_key passphrase_key = derive_passphrase(crypt, 
>>>> passphrase);
>>>>   @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>           memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>>>       }
>>>>   }
>>>> +
>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>>>> +        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>> +        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>> +        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>> +        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>> +}
>>>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>>>> index baea6d86..846a8931 100644
>>>> --- a/c_src/crypto.h
>>>> +++ b/c_src/crypto.h
>>>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, const 
>>>> char *,
>>>>   void bch2_add_key(struct bch_sb *, const char *, const char *, 
>>>> const char *);
>>>>   void bch_sb_crypt_init(struct bch_sb *sb, struct 
>>>> bch_sb_field_crypt *,
>>>>                  const char *);
>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>>>     #endif /* _CRYPTO_H */
>>>> -- 
>>>> 2.45.2
>>>>
>>>
>>>
>>

Re: [PATCH] Extract bch_crypt_update_passphrase function

[Hongbo Li]

On 2024/8/14 1:37, Mae Kasza wrote:
> This also fixes a bug where the set-passphrase
> command failed to derive the key for disks initially
> formatted with --encrypted and --no_passphrase, due to
> bch_sb_crypt_init not configuring the KDF params if
> the passphrase wasn't specified during formatting.
> 
> Signed-off-by: Mae Kasza <git@badat.dev>
> ---
>   c_src/cmd_key.c | 26 ++++++++++----------------
>   c_src/crypto.c  | 43 ++++++++++++++++++++++++++++++++-----------
>   c_src/crypto.h  |  3 +++
>   3 files changed, 45 insertions(+), 27 deletions(-)
> 
> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
> index adb0ac8d..ac8a94a8 100644
> --- a/c_src/cmd_key.c
> +++ b/c_src/cmd_key.c
> @@ -104,24 +104,19 @@ int cmd_set_passphrase(int argc, char *argv[])
>   	if (IS_ERR(c))
>   		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
>   
> -	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
> +	struct bch_sb *sb = c->disk_sb.sb;
> +	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>   	if (!crypt)
>   		die("Filesystem does not have encryption enabled");
>   
> -	struct bch_encrypted_key new_key;
> -	new_key.magic = BCH_KEY_MAGIC;
> -
> -	int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
> +	struct bch_key key;
> +	int ret = bch2_decrypt_sb_key(c, crypt, &key);
>   	if (ret)
>   		die("Error getting current key");
>   
>   	char *new_passphrase = read_passphrase_twice("Enter new passphrase: ");
> -	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
>   
> -	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(c->disk_sb.sb),
> -				    &new_key, sizeof(new_key)))
> -		die("error encrypting key");
> -	crypt->key = new_key;
> +	bch_crypt_update_passphrase(sb, crypt, &key, new_passphrase);
>   
>   	bch2_revoke_key(c->disk_sb.sb);
>   	bch2_write_super(c);
> @@ -142,18 +137,17 @@ int cmd_remove_passphrase(int argc, char *argv[])
>   	if (IS_ERR(c))
>   		die("Error opening %s: %s", argv[1], bch2_err_str(PTR_ERR(c)));
>   
> -	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(c->disk_sb.sb, crypt);
> +	struct bch_sb *sb = c->disk_sb.sb;
> +	struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>   	if (!crypt)
>   		die("Filesystem does not have encryption enabled");
>   
> -	struct bch_encrypted_key new_key;
> -	new_key.magic = BCH_KEY_MAGIC;
> -
> -	int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
> +	struct bch_key key;
> +	int ret = bch2_decrypt_sb_key(c, crypt, &key);
>   	if (ret)
>   		die("Error getting current key");
>   
> -	crypt->key = new_key;
> +	bch_crypt_update_passphrase(sb, crypt, &key, NULL);
>   
>   	bch2_write_super(c);
>   	bch2_fs_stop(c);
> diff --git a/c_src/crypto.c b/c_src/crypto.c
> index 32671bd8..301dbebe 100644
> --- a/c_src/crypto.c
> +++ b/c_src/crypto.c
> @@ -176,26 +176,47 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>   		       struct bch_sb_field_crypt *crypt,
>   		       const char *passphrase)
>   {
> +	struct bch_key key;
> +	get_random_bytes(&key, sizeof(key));
> +
>   	crypt->key.magic = BCH_KEY_MAGIC;
> -	get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
> +	crypt->key.key = key;
>   
> -	if (passphrase) {
> +	bch_crypt_update_passphrase(sb, crypt, &key, passphrase);
> +}
>   
> +void bch_crypt_update_passphrase(
> +			struct bch_sb *sb,
> +			struct bch_sb_field_crypt *crypt,
> +			struct bch_key *key,
> +			const char *new_passphrase)
> +{
> +
> +	struct bch_encrypted_key new_key;
> +	new_key.magic = BCH_KEY_MAGIC;
> +	new_key.key = *key;
> +
> +	if(!new_passphrase) {
> +		crypt->key = new_key;
May be the helper like this is needed. But we could assign the 
crypt->key outside, which seems more reasonable according to the helper 
name. Just in my humble opinion.

Thanks,
Hongbo

> +		return;
> +	}
> +
> +	// If crypt already has an encrypted key reuse it's encryption params
> +	if (!bch2_key_is_encrypted(&crypt->key)) {
>   		SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>   		SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>   		SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>   		SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
> +	}
>   
> -		struct bch_key passphrase_key = derive_passphrase(crypt, passphrase);
> -
> -		assert(!bch2_key_is_encrypted(&crypt->key));
> +	struct bch_key passphrase_key = derive_passphrase(crypt, new_passphrase);
>   
> -		if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
> -					   &crypt->key, sizeof(crypt->key)))
> -			die("error encrypting key");
> +	if (bch2_chacha_encrypt_key(&passphrase_key, __bch2_sb_key_nonce(sb),
> +				    &new_key, sizeof(new_key)))
> +		die("error encrypting key");
>   
> -		assert(bch2_key_is_encrypted(&crypt->key));
> +	memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>   
> -		memzero_explicit(&passphrase_key, sizeof(passphrase_key));
> -	}
> +	crypt->key = new_key;
> +	assert(bch2_key_is_encrypted(&crypt->key));
>   }
> diff --git a/c_src/crypto.h b/c_src/crypto.h
> index baea6d86..cd3baac8 100644
> --- a/c_src/crypto.h
> +++ b/c_src/crypto.h
> @@ -19,4 +19,7 @@ void bch2_add_key(struct bch_sb *, const char *, const char *, const char *);
>   void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
>   		       const char *);
>   
> +void bch_crypt_update_passphrase(struct bch_sb *sb, struct bch_sb_field_crypt *crypt,
> +			struct bch_key *key, const char *new_passphrase);
> +
>   #endif /* _CRYPTO_H */

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Mae Kasza]

On 8/14/24 04:10, Hongbo Li wrote:
>
>
> On 2024/8/13 21:16, Mae Kasza wrote:
>>
>> On 8/12/24 04:50, Hongbo Li wrote:
>>>
>>>
>>> On 2024/8/12 9:41, Kent Overstreet wrote:
>>>> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>>>>> From: mae <git@badat.dev>
>>>>>
>>>>> The set-passphrase command failed to derive the key for disks 
>>>>> initially
>>>>> formatted with --encrypted and --no_passphrase.
>>>>>
>>>>> This happened because bch_sb_crypt_init only configures the KDF 
>>>>> params
>>>>> if a passphrase is specified.
>>>>> This commit makes the command initialize the KDF with the same 
>>>>> parameters
>>>>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>>>>
>>>>> Signed-off-by: Mae Kasza <git@badat.dev>
>>>>
>>>> This looks good to me, but I'm in the middle of a 20 hour drive and 
>>>> not
>>>> feeling super confident to review crypto stuff tonight, so maybe 
>>>> someone
>>>> else can go over it too before I pull it in..
>>>>
>>>>> ---
>>>>>   c_src/cmd_key.c |  9 +++++++--
>>>>>   c_src/crypto.c  | 13 ++++++++-----
>>>>>   c_src/crypto.h  |  1 +
>>>>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>>>>> index adb0ac8d..2da83758 100644
>>>>> --- a/c_src/cmd_key.c
>>>>> +++ b/c_src/cmd_key.c
>>>>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>>       if (IS_ERR(c))
>>>>>           die("Error opening %s: %s", argv[1], 
>>>>> bch2_err_str(PTR_ERR(c)));
>>>>>   -    struct bch_sb_field_crypt *crypt = 
>>>>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>>>>> +    struct bch_sb *sb = c->disk_sb.sb;
>>>>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>>>>       if (!crypt)
>>>>>           die("Filesystem does not have encryption enabled");
>>>>>   @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>>           die("Error getting current key");
>>>>>         char *new_passphrase = read_passphrase_twice("Enter new 
>>>>> passphrase: ");
>>>>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>>>> +        bch_crypt_default_kdf_init(crypt);
>>> It works, but using bch_sb_crypt_init to reinitialize crypt may be 
>>> better. In bch_sb_crypt_init, it has initialized the crypt and also 
>>> cleaned new_passphrase.
>>>
>>> Thanks
>>> Hongbo
>>>
>> bch_sb_crypt_init also reinitializes the crypt->key, so I can't just 
>> reuse it here.
>>
> Yeah, it will change crypt->key. It seems the main point is not clean 
> the password.
> Do we need keep the crypt->key even the password is changed? Or does 
> the crypt->key like a random seed and need to be kept in the whole 
> lifecycle? Sorry I am a bit confused about this.
>
> Thanks,
> Hongbo

crypt->key does need to stay constant for the entire lifetime of a 
partition, since that's what's actually used to encrypt the data on the 
partition. The passphrase is just used to encrypt the crypt->key

Thanks,

Mae

>
>> I could extract it into a bch_crypt_update_passphrase function. 
>> Originally I wanted to avoid doing that to limit the scope of this 
>> patchset, but I'll give it a shot
>>
>> Thanks for the review!
>>
>> Mae
>>
>>>>> +    }
>>>>> +
>>>>>       struct bch_key passphrase_key = derive_passphrase(crypt, 
>>>>> new_passphrase);
>>>>>   -    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>>> __bch2_sb_key_nonce(c->disk_sb.sb),
>>>>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>>> __bch2_sb_key_nonce(sb),
>>>>>                       &new_key, sizeof(new_key)))
>>>>>           die("error encrypting key");
>>>>>       crypt->key = new_key;
>>>>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>>>>> index 32671bd8..30ad92d4 100644
>>>>> --- a/c_src/crypto.c
>>>>> +++ b/c_src/crypto.c
>>>>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>>       get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>>>>         if (passphrase) {
>>>>> -
>>>>> -        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>>> -        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>>> -        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>>> -        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>>> +        bch_crypt_default_kdf_init(crypt);
>>>>>             struct bch_key passphrase_key = 
>>>>> derive_passphrase(crypt, passphrase);
>>>>>   @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>>           memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>>>>       }
>>>>>   }
>>>>> +
>>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>>>>> +        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>>> +        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>>> +        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>>> +        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>>> +}
>>>>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>>>>> index baea6d86..846a8931 100644
>>>>> --- a/c_src/crypto.h
>>>>> +++ b/c_src/crypto.h
>>>>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, 
>>>>> const char *,
>>>>>   void bch2_add_key(struct bch_sb *, const char *, const char *, 
>>>>> const char *);
>>>>>   void bch_sb_crypt_init(struct bch_sb *sb, struct 
>>>>> bch_sb_field_crypt *,
>>>>>                  const char *);
>>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>>>>     #endif /* _CRYPTO_H */
>>>>> -- 
>>>>> 2.45.2
>>>>>
>>>>
>>>>
>>>

Re: [PATCH] Extract bch_crypt_update_passphrase function

[Mae Kasza]

On 8/14/24 04:21, Hongbo Li wrote:
>
>
> On 2024/8/14 1:37, Mae Kasza wrote:
>> This also fixes a bug where the set-passphrase
>> command failed to derive the key for disks initially
>> formatted with --encrypted and --no_passphrase, due to
>> bch_sb_crypt_init not configuring the KDF params if
>> the passphrase wasn't specified during formatting.
>>
>> Signed-off-by: Mae Kasza <git@badat.dev>
>> ---
>>   c_src/cmd_key.c | 26 ++++++++++----------------
>>   c_src/crypto.c  | 43 ++++++++++++++++++++++++++++++++-----------
>>   c_src/crypto.h  |  3 +++
>>   3 files changed, 45 insertions(+), 27 deletions(-)
>>
>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>> index adb0ac8d..ac8a94a8 100644
>> --- a/c_src/cmd_key.c
>> +++ b/c_src/cmd_key.c
>> @@ -104,24 +104,19 @@ int cmd_set_passphrase(int argc, char *argv[])
>>       if (IS_ERR(c))
>>           die("Error opening %s: %s", argv[1], 
>> bch2_err_str(PTR_ERR(c)));
>>   -    struct bch_sb_field_crypt *crypt = 
>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>> +    struct bch_sb *sb = c->disk_sb.sb;
>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>       if (!crypt)
>>           die("Filesystem does not have encryption enabled");
>>   -    struct bch_encrypted_key new_key;
>> -    new_key.magic = BCH_KEY_MAGIC;
>> -
>> -    int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
>> +    struct bch_key key;
>> +    int ret = bch2_decrypt_sb_key(c, crypt, &key);
>>       if (ret)
>>           die("Error getting current key");
>>         char *new_passphrase = read_passphrase_twice("Enter new 
>> passphrase: ");
>> -    struct bch_key passphrase_key = derive_passphrase(crypt, 
>> new_passphrase);
>>   -    if (bch2_chacha_encrypt_key(&passphrase_key, 
>> __bch2_sb_key_nonce(c->disk_sb.sb),
>> -                    &new_key, sizeof(new_key)))
>> -        die("error encrypting key");
>> -    crypt->key = new_key;
>> +    bch_crypt_update_passphrase(sb, crypt, &key, new_passphrase);
>>         bch2_revoke_key(c->disk_sb.sb);
>>       bch2_write_super(c);
>> @@ -142,18 +137,17 @@ int cmd_remove_passphrase(int argc, char *argv[])
>>       if (IS_ERR(c))
>>           die("Error opening %s: %s", argv[1], 
>> bch2_err_str(PTR_ERR(c)));
>>   -    struct bch_sb_field_crypt *crypt = 
>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>> +    struct bch_sb *sb = c->disk_sb.sb;
>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, crypt);
>>       if (!crypt)
>>           die("Filesystem does not have encryption enabled");
>>   -    struct bch_encrypted_key new_key;
>> -    new_key.magic = BCH_KEY_MAGIC;
>> -
>> -    int ret = bch2_decrypt_sb_key(c, crypt, &new_key.key);
>> +    struct bch_key key;
>> +    int ret = bch2_decrypt_sb_key(c, crypt, &key);
>>       if (ret)
>>           die("Error getting current key");
>>   -    crypt->key = new_key;
>> +    bch_crypt_update_passphrase(sb, crypt, &key, NULL);
>>         bch2_write_super(c);
>>       bch2_fs_stop(c);
>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>> index 32671bd8..301dbebe 100644
>> --- a/c_src/crypto.c
>> +++ b/c_src/crypto.c
>> @@ -176,26 +176,47 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>                  struct bch_sb_field_crypt *crypt,
>>                  const char *passphrase)
>>   {
>> +    struct bch_key key;
>> +    get_random_bytes(&key, sizeof(key));
>> +
>>       crypt->key.magic = BCH_KEY_MAGIC;
>> -    get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>> +    crypt->key.key = key;
>>   -    if (passphrase) {
>> +    bch_crypt_update_passphrase(sb, crypt, &key, passphrase);
>> +}
>>   +void bch_crypt_update_passphrase(
>> +            struct bch_sb *sb,
>> +            struct bch_sb_field_crypt *crypt,
>> +            struct bch_key *key,
>> +            const char *new_passphrase)
>> +{
>> +
>> +    struct bch_encrypted_key new_key;
>> +    new_key.magic = BCH_KEY_MAGIC;
>> +    new_key.key = *key;
>> +
>> +    if(!new_passphrase) {
>> +        crypt->key = new_key;
> May be the helper like this is needed. But we could assign the 
> crypt->key outside, which seems more reasonable according to the 
> helper name. Just in my humble opinion.
>
> Thanks,
> Hongbo

We need to assign to crypt->key regardless, since it's what contains the 
key encrypted with passphrase, I don't really see why we would want to 
leave that up to the caller

Thanks,

Mae

>
>> +        return;
>> +    }
>> +
>> +    // If crypt already has an encrypted key reuse it's encryption 
>> params
>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>           SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>           SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>           SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>           SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>> +    }
>>   -        struct bch_key passphrase_key = derive_passphrase(crypt, 
>> passphrase);
>> -
>> -        assert(!bch2_key_is_encrypted(&crypt->key));
>> +    struct bch_key passphrase_key = derive_passphrase(crypt, 
>> new_passphrase);
>>   -        if (bch2_chacha_encrypt_key(&passphrase_key, 
>> __bch2_sb_key_nonce(sb),
>> -                       &crypt->key, sizeof(crypt->key)))
>> -            die("error encrypting key");
>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>> __bch2_sb_key_nonce(sb),
>> +                    &new_key, sizeof(new_key)))
>> +        die("error encrypting key");
>>   -        assert(bch2_key_is_encrypted(&crypt->key));
>> +    memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>   -        memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>> -    }
>> +    crypt->key = new_key;
>> +    assert(bch2_key_is_encrypted(&crypt->key));
>>   }
>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>> index baea6d86..cd3baac8 100644
>> --- a/c_src/crypto.h
>> +++ b/c_src/crypto.h
>> @@ -19,4 +19,7 @@ void bch2_add_key(struct bch_sb *, const char *, 
>> const char *, const char *);
>>   void bch_sb_crypt_init(struct bch_sb *sb, struct bch_sb_field_crypt *,
>>                  const char *);
>>   +void bch_crypt_update_passphrase(struct bch_sb *sb, struct 
>> bch_sb_field_crypt *crypt,
>> +            struct bch_key *key, const char *new_passphrase);
>> +
>>   #endif /* _CRYPTO_H */

Re: [PATCH] cmd_set_passphrase: initialize KDF parameters

[Mae Kasza]

Hi,

Appologies if this is bad etiquette, but I haven't gotten any review 
feedback and it seems like fixing this is relatively important to 
users(https://github.com/koverstreet/bcachefs-tools/pull/329#issuecomment-2336787443). 
Any chance someone(Kent) could look at this and merge the patch?

Thanks,

Mae

On 8/14/24 14:25, Mae Kasza wrote:
>
> On 8/14/24 04:10, Hongbo Li wrote:
>>
>>
>> On 2024/8/13 21:16, Mae Kasza wrote:
>>>
>>> On 8/12/24 04:50, Hongbo Li wrote:
>>>>
>>>>
>>>> On 2024/8/12 9:41, Kent Overstreet wrote:
>>>>> On Sun, Aug 11, 2024 at 11:40:38PM GMT, Mae Kasza wrote:
>>>>>> From: mae <git@badat.dev>
>>>>>>
>>>>>> The set-passphrase command failed to derive the key for disks 
>>>>>> initially
>>>>>> formatted with --encrypted and --no_passphrase.
>>>>>>
>>>>>> This happened because bch_sb_crypt_init only configures the KDF 
>>>>>> params
>>>>>> if a passphrase is specified.
>>>>>> This commit makes the command initialize the KDF with the same 
>>>>>> parameters
>>>>>> as bch_sb_crypt_init if the key wasn't encrypted before.
>>>>>>
>>>>>> Signed-off-by: Mae Kasza <git@badat.dev>
>>>>>
>>>>> This looks good to me, but I'm in the middle of a 20 hour drive 
>>>>> and not
>>>>> feeling super confident to review crypto stuff tonight, so maybe 
>>>>> someone
>>>>> else can go over it too before I pull it in..
>>>>>
>>>>>> ---
>>>>>>   c_src/cmd_key.c |  9 +++++++--
>>>>>>   c_src/crypto.c  | 13 ++++++++-----
>>>>>>   c_src/crypto.h  |  1 +
>>>>>>   3 files changed, 16 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/c_src/cmd_key.c b/c_src/cmd_key.c
>>>>>> index adb0ac8d..2da83758 100644
>>>>>> --- a/c_src/cmd_key.c
>>>>>> +++ b/c_src/cmd_key.c
>>>>>> @@ -104,7 +104,8 @@ int cmd_set_passphrase(int argc, char *argv[])
>>>>>>       if (IS_ERR(c))
>>>>>>           die("Error opening %s: %s", argv[1], 
>>>>>> bch2_err_str(PTR_ERR(c)));
>>>>>>   -    struct bch_sb_field_crypt *crypt = 
>>>>>> bch2_sb_field_get(c->disk_sb.sb, crypt);
>>>>>> +    struct bch_sb *sb = c->disk_sb.sb;
>>>>>> +    struct bch_sb_field_crypt *crypt = bch2_sb_field_get(sb, 
>>>>>> crypt);
>>>>>>       if (!crypt)
>>>>>>           die("Filesystem does not have encryption enabled");
>>>>>>   @@ -116,9 +117,13 @@ int cmd_set_passphrase(int argc, char 
>>>>>> *argv[])
>>>>>>           die("Error getting current key");
>>>>>>         char *new_passphrase = read_passphrase_twice("Enter new 
>>>>>> passphrase: ");
>>>>>> +    if (!bch2_key_is_encrypted(&crypt->key)) {
>>>>>> +        bch_crypt_default_kdf_init(crypt);
>>>> It works, but using bch_sb_crypt_init to reinitialize crypt may be 
>>>> better. In bch_sb_crypt_init, it has initialized the crypt and also 
>>>> cleaned new_passphrase.
>>>>
>>>> Thanks
>>>> Hongbo
>>>>
>>> bch_sb_crypt_init also reinitializes the crypt->key, so I can't just 
>>> reuse it here.
>>>
>> Yeah, it will change crypt->key. It seems the main point is not clean 
>> the password.
>> Do we need keep the crypt->key even the password is changed? Or does 
>> the crypt->key like a random seed and need to be kept in the whole 
>> lifecycle? Sorry I am a bit confused about this.
>>
>> Thanks,
>> Hongbo
>
> crypt->key does need to stay constant for the entire lifetime of a 
> partition, since that's what's actually used to encrypt the data on 
> the partition. The passphrase is just used to encrypt the crypt->key
>
> Thanks,
>
> Mae
>
>>
>>> I could extract it into a bch_crypt_update_passphrase function. 
>>> Originally I wanted to avoid doing that to limit the scope of this 
>>> patchset, but I'll give it a shot
>>>
>>> Thanks for the review!
>>>
>>> Mae
>>>
>>>>>> +    }
>>>>>> +
>>>>>>       struct bch_key passphrase_key = derive_passphrase(crypt, 
>>>>>> new_passphrase);
>>>>>>   -    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>>>> __bch2_sb_key_nonce(c->disk_sb.sb),
>>>>>> +    if (bch2_chacha_encrypt_key(&passphrase_key, 
>>>>>> __bch2_sb_key_nonce(sb),
>>>>>>                       &new_key, sizeof(new_key)))
>>>>>>           die("error encrypting key");
>>>>>>       crypt->key = new_key;
>>>>>> diff --git a/c_src/crypto.c b/c_src/crypto.c
>>>>>> index 32671bd8..30ad92d4 100644
>>>>>> --- a/c_src/crypto.c
>>>>>> +++ b/c_src/crypto.c
>>>>>> @@ -180,11 +180,7 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>>>       get_random_bytes(&crypt->key.key, sizeof(crypt->key.key));
>>>>>>         if (passphrase) {
>>>>>> -
>>>>>> -        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>>>> -        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>>>> -        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>>>> -        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>>>> +        bch_crypt_default_kdf_init(crypt);
>>>>>>             struct bch_key passphrase_key = 
>>>>>> derive_passphrase(crypt, passphrase);
>>>>>>   @@ -199,3 +195,10 @@ void bch_sb_crypt_init(struct bch_sb *sb,
>>>>>>           memzero_explicit(&passphrase_key, sizeof(passphrase_key));
>>>>>>       }
>>>>>>   }
>>>>>> +
>>>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *crypt) {
>>>>>> +        SET_BCH_CRYPT_KDF_TYPE(crypt, BCH_KDF_SCRYPT);
>>>>>> +        SET_BCH_KDF_SCRYPT_N(crypt, ilog2(16384));
>>>>>> +        SET_BCH_KDF_SCRYPT_R(crypt, ilog2(8));
>>>>>> +        SET_BCH_KDF_SCRYPT_P(crypt, ilog2(16));
>>>>>> +}
>>>>>> diff --git a/c_src/crypto.h b/c_src/crypto.h
>>>>>> index baea6d86..846a8931 100644
>>>>>> --- a/c_src/crypto.h
>>>>>> +++ b/c_src/crypto.h
>>>>>> @@ -18,5 +18,6 @@ void bch2_passphrase_check(struct bch_sb *, 
>>>>>> const char *,
>>>>>>   void bch2_add_key(struct bch_sb *, const char *, const char *, 
>>>>>> const char *);
>>>>>>   void bch_sb_crypt_init(struct bch_sb *sb, struct 
>>>>>> bch_sb_field_crypt *,
>>>>>>                  const char *);
>>>>>> +void bch_crypt_default_kdf_init(struct bch_sb_field_crypt *);
>>>>>>     #endif /* _CRYPTO_H */
>>>>>> -- 
>>>>>> 2.45.2
>>>>>>
>>>>>
>>>>>
>>>>