linux-kernel - Re: [PATCH v1 RESEND] nvmem: layouts: u-boot-env: remove crc32 endianness conversion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <206d1405-5a8b-4f2a-8545-458c88345a41@linaro.org>
Date: Tue, 8 Apr 2025 08:52:51 +0100
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: "Michael C. Pratt" <mcpratt@...me>
Cc: Christian Lamparter <chunkeey@...il.com>, Rafał Miłecki
 <rafal@...ecki.pl>, linux-kernel@...r.kernel.org,
 INAGAKI Hiroshi <musashino.open@...il.com>
Subject: Re: [PATCH v1 RESEND] nvmem: layouts: u-boot-env: remove crc32
 endianness conversion



On 05/04/2025 05:57, Michael C. Pratt wrote:
> On 11 Oct 2022, it was reported that the crc32 verification
> of the u-boot environment failed only on big-endian systems
> for the u-boot-env nvmem layout driver with the following error.
> 
>    Invalid calculated CRC32: 0x88cd6f09 (expected: 0x096fcd88)
> 
> This problem has been present since the driver was introduced,
> and before it was made into a layout driver.
> 
> The suggested fix at the time was to use further endianness
> conversion macros in order to have both the stored and calculated
> crc32 values to compare always represented in the system's endianness.
> This was not accepted due to sparse warnings
> and some disagreement on how to handle the situation.
> Later on in a newer revision of the patch, it was proposed to use
> cpu_to_le32() for both values to compare instead of le32_to_cpu()
> and store the values as __le32 type to remove compilation errors.
> 
> The necessity of this is based on the assumption that the use of crc32()
> requires endianness conversion because the algorithm uses little-endian,
> however, this does not prove to be the case and the issue is unrelated.
> 
> Upon inspecting the current kernel code,
> there already is an existing use of le32_to_cpu() in this driver,
> which suggests there already is special handling for big-endian systems,
> however, it is big-endian systems that have the problem.
> 
> This, being the only functional difference between architectures
> in the driver combined with the fact that the suggested fix
> was to use the exact same endianness conversion for the values
> brings up the possibility that it was not necessary to begin with,
> as the same endianness conversion for two values expected to be the same
> is expected to be equivalent to no conversion at all.
> 
> After inspecting the u-boot environment of devices of both endianness
> and trying to remove the existing endianness conversion,
> the problem is resolved in an equivalent way as the other suggested fixes.
> 
> Ultimately, it seems that u-boot is agnostic to endianness
> at least for the purpose of environment variables.
> In other words, u-boot reads and writes the stored crc32 value
> with the same endianness that the crc32 value is calculated with
> in whichever endianness a certain architecture runs on.
> 
> Therefore, the u-boot-env driver does not need to convert endianness.
> Remove the usage of endianness macros in the u-boot-env driver,
> and change the type of local variables to maintain the same return type.
> 
> If there is a special situation in the case of endianness,
> it would be a corner case and should be handled by a unique "compatible".
> 
> Even though it is not necessary to use endianness conversion macros here,
> it may be useful to use them in the future for consistent error printing.

> 
> Fixes: d5542923f200 ("nvmem: add driver handling U-Boot environment variables")
> Reported-by: INAGAKI Hiroshi <musashino.open@...il.com>

CC Stable is missing.

--srini
> Link: https://lore.kernel.org/all/20221011024928.1807-1-musashino.open@gmail.com
> Signed-off-by: Michael C. Pratt <mcpratt@...me>
> ---
>   drivers/nvmem/layouts/u-boot-env.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvmem/layouts/u-boot-env.c b/drivers/nvmem/layouts/u-boot-env.c
> index 731e6f4f12b2..21f6dcf905dd 100644
> --- a/drivers/nvmem/layouts/u-boot-env.c
> +++ b/drivers/nvmem/layouts/u-boot-env.c
> @@ -92,7 +92,7 @@ int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
>   	size_t crc32_data_offset;
>   	size_t crc32_data_len;
>   	size_t crc32_offset;
> -	__le32 *crc32_addr;
> +	uint32_t *crc32_addr;
>   	size_t data_offset;
>   	size_t data_len;
>   	size_t dev_size;
> @@ -143,8 +143,8 @@ int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
>   		goto err_kfree;
>   	}
>   
> -	crc32_addr = (__le32 *)(buf + crc32_offset);
> -	crc32 = le32_to_cpu(*crc32_addr);
> +	crc32_addr = (uint32_t *)(buf + crc32_offset);
> +	crc32 = *crc32_addr;
>   	crc32_data_len = dev_size - crc32_data_offset;
>   	data_len = dev_size - data_offset;
>   
> 
> base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557