[PATCH master 1/3] common: machine_id: simplify early exit

[PATCH master 1/3] common: machine_id: simplify early exit

[Ahmad Fatoum]

We don't need the goto if we haven't done anything to clean up anyway.
also globalvar_add_simple("machine_id", NULL) is a no-op when we have
just called globalvar_add_simple above with an actual argument.
It doesn't clean the parameter, nor should it, because the code is
executed for the successful code as well and there is nothing that can
fail that late.

No functional change.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/machine_id.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/common/machine_id.c b/common/machine_id.c
index 6480806cd287..a530fdeb1da8 100644
--- a/common/machine_id.c
+++ b/common/machine_id.c
@@ -30,11 +30,11 @@ static int machine_id_set_globalvar(void)
 	unsigned char machine_id[SHA1_DIGEST_SIZE];
 	char hex_machine_id[MACHINE_ID_LENGTH];
 	char *env_machine_id;
-	int ret = 0;
+	int ret;
 
 	/* nothing to do if no hashable information provided */
 	if (!__machine_id_hashable)
-		goto out;
+		return 0;
 
 	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
 	ret = digest_init(digest);
@@ -58,8 +58,6 @@ static int machine_id_set_globalvar(void)
 	free(env_machine_id);
 
 out:
-	globalvar_add_simple("machine_id", NULL);
-
 	digest_free(digest);
 	return ret;
 
-- 
2.30.2

[PATCH master 2/3] common: machine_id: guard against digest algo being unavailable

[Ahmad Fatoum]

All callsites of digest_alloc(_by_algo) check for NULL pointer before
proceeding except for machine_id_set_globalvar(). Fix this to fail
more gracefully instead of the crash I ran into.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/machine_id.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/machine_id.c b/common/machine_id.c
index a530fdeb1da8..189e74bd6796 100644
--- a/common/machine_id.c
+++ b/common/machine_id.c
@@ -30,14 +30,15 @@ static int machine_id_set_globalvar(void)
 	unsigned char machine_id[SHA1_DIGEST_SIZE];
 	char hex_machine_id[MACHINE_ID_LENGTH];
 	char *env_machine_id;
-	int ret;
+	int ret = -EOPNOTSUPP;
 
 	/* nothing to do if no hashable information provided */
 	if (!__machine_id_hashable)
 		return 0;
 
 	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
-	ret = digest_init(digest);
+	if (digest)
+		ret = digest_init(digest);
 	if (ret)
 		goto out;
 
-- 
2.30.2

[PATCH master 3/3] crypto: restrict digest algos implemented in ARM assembly to 32-bit

[Ahmad Fatoum]

These digest implementations are only built per arch/arm/Makefile when
!CONFIG_CPU_V8, so disallow selecting them when barebox is built
for 64-bit ARM.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 crypto/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index a2e03ae1096e..f32accb3d090 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -71,7 +71,7 @@ config DIGEST_HMAC_GENERIC
 
 config DIGEST_SHA1_ARM
 	tristate "SHA1 digest algorithm (ARM-asm)"
-	depends on ARM
+	depends on ARM && !CPU_V8
 	select HAVE_DIGEST_SHA1
 	help
 	  SHA-1 secure hash standard (FIPS 180-1/DFIPS 180-2) implemented
@@ -79,7 +79,7 @@ config DIGEST_SHA1_ARM
 
 config DIGEST_SHA256_ARM
 	tristate "SHA-224/256 digest algorithm (ARM-asm and NEON)"
-	depends on ARM
+	depends on ARM && !CPU_V8
 	select HAVE_DIGEST_SHA256
 	select HAVE_DIGEST_SHA224
 	help
-- 
2.30.2

Re: [PATCH master 1/3] common: machine_id: simplify early exit

[Sascha Hauer]

On Mon, Aug 08, 2022 at 08:20:08AM +0200, Ahmad Fatoum wrote:
> We don't need the goto if we haven't done anything to clean up anyway.
> also globalvar_add_simple("machine_id", NULL) is a no-op when we have
> just called globalvar_add_simple above with an actual argument.
> It doesn't clean the parameter, nor should it, because the code is
> executed for the successful code as well and there is nothing that can
> fail that late.
> 
> No functional change.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/machine_id.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/common/machine_id.c b/common/machine_id.c
> index 6480806cd287..a530fdeb1da8 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -30,11 +30,11 @@ static int machine_id_set_globalvar(void)
>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>  	char hex_machine_id[MACHINE_ID_LENGTH];
>  	char *env_machine_id;
> -	int ret = 0;
> +	int ret;
>  
>  	/* nothing to do if no hashable information provided */
>  	if (!__machine_id_hashable)
> -		goto out;
> +		return 0;
>  
>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
>  	ret = digest_init(digest);
> @@ -58,8 +58,6 @@ static int machine_id_set_globalvar(void)
>  	free(env_machine_id);
>  
>  out:
> -	globalvar_add_simple("machine_id", NULL);

Without this patch we always created a global.machine_id variable,
either empty or with a value. With this patch we only create this
variable when it's non-empty. I think this change is ok as we also
don't have this variable when CONFIG_MACHINE_ID is disabled.

Still the "No functional change" doesn't hold true, so we should remove
it from the commit message.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 1/3] common: machine_id: simplify early exit

[Ahmad Fatoum]

On 08.08.22 13:40, Sascha Hauer wrote:
> On Mon, Aug 08, 2022 at 08:20:08AM +0200, Ahmad Fatoum wrote:
>> We don't need the goto if we haven't done anything to clean up anyway.
>> also globalvar_add_simple("machine_id", NULL) is a no-op when we have
>> just called globalvar_add_simple above with an actual argument.
>> It doesn't clean the parameter, nor should it, because the code is
>> executed for the successful code as well and there is nothing that can
>> fail that late.
>>
>> No functional change.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/machine_id.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/common/machine_id.c b/common/machine_id.c
>> index 6480806cd287..a530fdeb1da8 100644
>> --- a/common/machine_id.c
>> +++ b/common/machine_id.c
>> @@ -30,11 +30,11 @@ static int machine_id_set_globalvar(void)
>>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>>  	char hex_machine_id[MACHINE_ID_LENGTH];
>>  	char *env_machine_id;
>> -	int ret = 0;
>> +	int ret;
>>  
>>  	/* nothing to do if no hashable information provided */
>>  	if (!__machine_id_hashable)
>> -		goto out;
>> +		return 0;
>>  
>>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
>>  	ret = digest_init(digest);
>> @@ -58,8 +58,6 @@ static int machine_id_set_globalvar(void)
>>  	free(env_machine_id);
>>  
>>  out:
>> -	globalvar_add_simple("machine_id", NULL);
> 
> Without this patch we always created a global.machine_id variable,
> either empty or with a value. With this patch we only create this
> variable when it's non-empty. I think this change is ok as we also
> don't have this variable when CONFIG_MACHINE_ID is disabled.
> 
> Still the "No functional change" doesn't hold true, so we should remove
> it from the commit message.

You're right. I missed that. Can you replace the last line with:

  This slightly alters behavior: Whereas before $global.machine_id was
  always defined when CONFIG_MACHINE_ID is enabled, it's now only
  defined when it's non-empty.

Thanks,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 2/3] common: machine_id: guard against digest algo being unavailable

[Sascha Hauer]

On Mon, Aug 08, 2022 at 08:20:09AM +0200, Ahmad Fatoum wrote:
> All callsites of digest_alloc(_by_algo) check for NULL pointer before
> proceeding except for machine_id_set_globalvar(). Fix this to fail
> more gracefully instead of the crash I ran into.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/machine_id.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/common/machine_id.c b/common/machine_id.c
> index a530fdeb1da8..189e74bd6796 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -30,14 +30,15 @@ static int machine_id_set_globalvar(void)
>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>  	char hex_machine_id[MACHINE_ID_LENGTH];
>  	char *env_machine_id;
> -	int ret;
> +	int ret = -EOPNOTSUPP;
>  
>  	/* nothing to do if no hashable information provided */
>  	if (!__machine_id_hashable)
>  		return 0;
>  
>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);

Had to read this twice to be sure it's correct. A plain

	if (!digest) {
		ret = -EOPNOTSUPP;
		goto out;
	}

seems easier to follow.

Sascha

> -	ret = digest_init(digest);
> +	if (digest)
> +		ret = digest_init(digest);
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 2/3] common: machine_id: guard against digest algo being unavailable

[Ahmad Fatoum]

On 08.08.22 13:48, Sascha Hauer wrote:
> On Mon, Aug 08, 2022 at 08:20:09AM +0200, Ahmad Fatoum wrote:
>> All callsites of digest_alloc(_by_algo) check for NULL pointer before
>> proceeding except for machine_id_set_globalvar(). Fix this to fail
>> more gracefully instead of the crash I ran into.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/machine_id.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/machine_id.c b/common/machine_id.c
>> index a530fdeb1da8..189e74bd6796 100644
>> --- a/common/machine_id.c
>> +++ b/common/machine_id.c
>> @@ -30,14 +30,15 @@ static int machine_id_set_globalvar(void)
>>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>>  	char hex_machine_id[MACHINE_ID_LENGTH];
>>  	char *env_machine_id;
>> -	int ret;
>> +	int ret = -EOPNOTSUPP;
>>  
>>  	/* nothing to do if no hashable information provided */
>>  	if (!__machine_id_hashable)
>>  		return 0;
>>  
>>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
> 
> Had to read this twice to be sure it's correct. A plain
> 
> 	if (!digest) {
> 		ret = -EOPNOTSUPP;
> 		goto out;
> 	}
> 
> seems easier to follow.

Fine by me. Can you fixup or should I?

Cheers,
Ahmad

> 
> Sascha
> 
>> -	ret = digest_init(digest);
>> +	if (digest)
>> +		ret = digest_init(digest);
>>  	if (ret)
>>  		goto out;
>>  
>> -- 
>> 2.30.2
>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master 1/3] common: machine_id: simplify early exit

[Sascha Hauer]

On Mon, Aug 08, 2022 at 08:20:08AM +0200, Ahmad Fatoum wrote:
> We don't need the goto if we haven't done anything to clean up anyway.
> also globalvar_add_simple("machine_id", NULL) is a no-op when we have
> just called globalvar_add_simple above with an actual argument.
> It doesn't clean the parameter, nor should it, because the code is
> executed for the successful code as well and there is nothing that can
> fail that late.
> 
> No functional change.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/machine_id.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/common/machine_id.c b/common/machine_id.c
> index 6480806cd287..a530fdeb1da8 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -30,11 +30,11 @@ static int machine_id_set_globalvar(void)
>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>  	char hex_machine_id[MACHINE_ID_LENGTH];
>  	char *env_machine_id;
> -	int ret = 0;
> +	int ret;
>  
>  	/* nothing to do if no hashable information provided */
>  	if (!__machine_id_hashable)
> -		goto out;
> +		return 0;
>  
>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
>  	ret = digest_init(digest);
> @@ -58,8 +58,6 @@ static int machine_id_set_globalvar(void)
>  	free(env_machine_id);
>  
>  out:
> -	globalvar_add_simple("machine_id", NULL);
> -
>  	digest_free(digest);
>  	return ret;
>  
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |