linux-kernel - Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable

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] [day] [month] [year] [list]

Message-ID: <1ace6102-2e88-499a-a5fd-71951863b987@redhat.com>
Date: Fri, 20 Dec 2024 14:39:18 -0500
From: Jennifer Berringer <jberring@...hat.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Sebastian Reichel <sre@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Maxime Ripard <mripard@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

Thanks for the feedback and for accepting my first patch.

On 12/14/24 10:07, Srinivas Kandagatla wrote:
>> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
> 
>> + *
>> + * @cell: nvmem cell to be written.
>> + * @val: Value to be written which may be truncated.
>> + *
>> + * Return: length of bytes written or negative on failure.
>> + */
>> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> 
> This new API is confusing, as writing to cell should in the same order of the data. Doing any data manipulation within the api is confusing to users.
> 
> If the intention is to know the size of cell before writing, then best way to address this is to provide the cell size visibility to the consumer.
> 
> --srini

My intention was to mirror the existing functions in this file, hoping that would make it less confusing rather than more. nvmem_cell_read_variable_le_u32() already similarly has consumers accessing the contents of a cell without knowing its size, silently filling any bytes not read with zero. The function I add doesn't change the order of the bytes. The existing read_variable_le functions (in contrast) byte swap from little-endian to the CPU's endianness and indicate this in the function name. Otherwise, I believe the function I add here is what would be expected from a write equivalent. Its return value also gives the caller the actual cell size upon success.

The only manipulation done to the data here is truncating to ignore the highest-order bytes. This behavior should match the below example unless the size is 3 bytes, which my patch should handle but the below example can't.

if (entry->bytes == sizeof(u32)) {
    return __nvmem_cell_entry_write(entry, &val, sizeof(u32));
} else if (entry->bytes == sizeof(u16)) {
    u16 val_16 = (u16) val;
    return __nvmem_cell_entry_write(entry, &val_16, sizeof(u16));
} else if (entry->bytes == sizeof(u8)) {
    u8 val_8 = (u8) val;
    return __nvmem_cell_entry_write(entry, &val_8, sizeof(u8));
} else {
    return -EINVAL;
}

Still, if you prefer, I can send new patches that add a function to get the cell size and put any truncation behavior needed by nvmem-reboot-mode into that source file instead.