linux-kernel - Re: [PATCH v3] nvmem: u-boot-env: align endianness of crc32 values

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1276b70d-9461-4a4c-08f7-2bed42557fd6@gmail.com>
Date:   Sun, 19 Feb 2023 13:45:33 +0900
From:   INAGAKI Hiroshi <musashino.open@...il.com>
To:     Christian Lamparter <chunkeey@...il.com>,
        Rafał Miłecki <rafal@...ecki.pl>
Cc:     srinivas.kandagatla@...aro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] nvmem: u-boot-env: align endianness of crc32 values

Hi Christian,

On 2023/02/18 2:30, Christian Lamparter wrote:
> On 2/13/23 14:37, Rafał Miłecki wrote:
>> On 2023-02-13 14:23, INAGAKI Hiroshi wrote:
>>> This patch fixes crc32 error on Big-Endianness system by 
>>> conversion of
>>> calculated crc32 value.
>>>
>>> Little-Endianness system:
>>>
>>>   obtained crc32: Little
>>> calculated crc32: Little
>>>
>>> Big-Endianness system:
>>>
>>>   obtained crc32: Little
>>> calculated crc32: Big
>>>
>>> log (APRESIA ApresiaLightGS120GT-SS, RTL8382M, Big-Endianness):
>>>
>>> [    8.570000] u_boot_env
>>> 18001200.spi:flash@0:partitions:partition@...00: Invalid calculated
>>> CRC32: 0x88cd6f09 (expected: 0x096fcd88)
>>> [    8.580000] u_boot_env: probe of
>>> 18001200.spi:flash@0:partitions:partition@...00 failed with error -22
>>>
>>> Fixes: f955dc144506 ("nvmem: add driver handling U-Boot 
>>> environment variables")
>>>
>>> Signed-off-by: INAGAKI Hiroshi <musashino.open@...il.com>
>>> ---
>>> v2 -> v3
>>>
>>> - fix sparse warning by using __le32 type and cpu_to_le32
>>> - fix character length of the short commit hash in "Fixes:" tag
>>>
>>> v1 -> v2
>>>
>>> - wrong fix for sparse warning due to misunderstanding
>>> - add missing "Fixes:" tag
>>>
>>>  drivers/nvmem/u-boot-env.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
>>> index 29b1d87a3c51..164bb04dfc3b 100644
>>> --- a/drivers/nvmem/u-boot-env.c
>>> +++ b/drivers/nvmem/u-boot-env.c
>>> @@ -117,8 +117,8 @@ static int u_boot_env_parse(struct u_boot_env 
>>> *priv)
>>>      size_t crc32_offset;
>>>      size_t data_offset;
>>>      size_t data_len;
>>> -    uint32_t crc32;
>>> -    uint32_t calc;
>>> +    __le32 crc32;
>>> +    __le32 calc;
>>>      size_t bytes;
>>>      uint8_t *buf;
>>>      int err;
>>
>> This looks counter-intuitive to me, to store values on host system in
>> specified endianness. I'd say we should use __le32 type only to
>> represent numbers in device stored data (e.g. structs as processed by
>> device).
>>
>> My suggesion: leave uint32_t for local variables and use 
>> le32_to_cpu().
>
> Hmm, this is strange. The kernel's u-boot-env driver works without any
> additional changes in the le<->be department on the Big-Endian
> PowerPC APM82181 WD MyBook Live NAS.
>
> Is there something odd going on with the WD MyBook Live, or is it
> the APRESIA ApresiaLightGS120GT-SS that is special?
>

This additional changes are for resolving sparse warnings. Of course 
it's working fine on my device with the previous changes, but due to 
the warning it wasn't merged into the mainline and needs to be resolved.

> Regards,
> Christian

Thanks,
Hiroshi