[dm-crypt] [PATCH] crypt_plain_hash: Remove dead code

[dm-crypt] [PATCH] crypt_plain_hash: Remove dead code

[Richard Weinberger]

Since commit 31a4d552a2d9 ("Support keyfile offset and keyfile size option even for plain volumes.")
params.hash is set to NULL when the requested hash function is
"plain". Therfore the code to handle the "plain" hasher in
crypt_plain_hash() is no longer reachable and can be removed.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 lib/crypt_plain.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/lib/crypt_plain.c b/lib/crypt_plain.c
index f89c7747649d..66339bdbf2c9 100644
--- a/lib/crypt_plain.c
+++ b/lib/crypt_plain.c
@@ -99,17 +99,7 @@ int crypt_plain_hash(struct crypt_device *cd,
 		pad_size = 0;
 	}
 
-	/* No hash, copy passphrase directly */
-	if (!strcmp(hash_name_buf, "plain")) {
-		if (passphrase_size < hash_size) {
-			log_dbg(cd, "Too short plain passphrase.");
-			return -EINVAL;
-		}
-		memcpy(key, passphrase, hash_size);
-		r = 0;
-	} else
-		r = hash(hash_name_buf, hash_size, key, passphrase_size, passphrase);
-
+	r = hash(hash_name_buf, hash_size, key, passphrase_size, passphrase);
 	if (r == 0 && pad_size)
 		memset(key + hash_size, 0, pad_size);
 
-- 
2.20.1

Re: [dm-crypt] [PATCH] crypt_plain_hash: Remove dead code

[Milan Broz]

On 18/01/2019 15:14, Richard Weinberger wrote:
> Since commit 31a4d552a2d9 ("Support keyfile offset and keyfile size option even for plain volumes.")
> params.hash is set to NULL when the requested hash function is
> "plain". Therfore the code to handle the "plain" hasher in
> crypt_plain_hash() is no longer reachable and can be removed.

Hi,

thanks for the patch, but unfortunately it is not the correct fix.

While cryptsetup set params.hash to NULL, other users can still use "plain" to indicate no hashing
(I think systemd-cryptsetup is one of them).

IOW: /lib/* is libcryptsetup API, your code modifies how the libcryptsetup API works now.
(Despite "plain" hash is legacy code, and I wish it to be removed...)

You referenced commit that modifies cryptsetup binary, cryptsetup is only one user of libcryptsetup API,
so the conclusion that the code is no longer used is misleading.

(My bad that there was no test for it in libcryptsetup api-test, I just added such a test to the git.)

Thanks,
Milan


> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  lib/crypt_plain.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/lib/crypt_plain.c b/lib/crypt_plain.c
> index f89c7747649d..66339bdbf2c9 100644
> --- a/lib/crypt_plain.c
> +++ b/lib/crypt_plain.c
> @@ -99,17 +99,7 @@ int crypt_plain_hash(struct crypt_device *cd,
>  		pad_size = 0;
>  	}
>  
> -	/* No hash, copy passphrase directly */
> -	if (!strcmp(hash_name_buf, "plain")) {
> -		if (passphrase_size < hash_size) {
> -			log_dbg(cd, "Too short plain passphrase.");
> -			return -EINVAL;
> -		}
> -		memcpy(key, passphrase, hash_size);
> -		r = 0;
> -	} else
> -		r = hash(hash_name_buf, hash_size, key, passphrase_size, passphrase);
> -
> +	r = hash(hash_name_buf, hash_size, key, passphrase_size, passphrase);
>  	if (r == 0 && pad_size)
>  		memset(key + hash_size, 0, pad_size);
>  
> 

Re: [dm-crypt] [PATCH] crypt_plain_hash: Remove dead code

[Richard Weinberger]

Milan,

Am Sonntag, 20. Januar 2019, 10:33:46 CET schrieb Milan Broz:
> On 18/01/2019 15:14, Richard Weinberger wrote:
> > Since commit 31a4d552a2d9 ("Support keyfile offset and keyfile size option even for plain volumes.")
> > params.hash is set to NULL when the requested hash function is
> > "plain". Therfore the code to handle the "plain" hasher in
> > crypt_plain_hash() is no longer reachable and can be removed.
> 
> Hi,
> 
> thanks for the patch, but unfortunately it is not the correct fix.
> 
> While cryptsetup set params.hash to NULL, other users can still use "plain" to indicate no hashing
> (I think systemd-cryptsetup is one of them).
> 
> IOW: /lib/* is libcryptsetup API, your code modifies how the libcryptsetup API works now.
> (Despite "plain" hash is legacy code, and I wish it to be removed...)
> 
> You referenced commit that modifies cryptsetup binary, cryptsetup is only one user of libcryptsetup API,
> so the conclusion that the code is no longer used is misleading.
> 
> (My bad that there was no test for it in libcryptsetup api-test, I just added such a test to the git.)

Ahhh, stupid me. I completely forgot that libcryptsetup is supposed to be used by
programs outside of the cryptsetup code base.

Thanks,
//richard