| 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
| ||
|
Message-ID: <ba8abaf6-6f16-4742-a3ea-ad00d4151698@gmail.com>
Date: Tue, 19 Dec 2023 10:55:04 +0100
From: Rafał Miłecki <zajec5@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Michael Walle <michael@...le.cc>, linux-mtd@...ts.infradead.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, u-boot@...ts.denx.de,
Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH 4/4] nvmem: layouts: add U-Boot env layout
On 19.12.2023 08:55, Miquel Raynal wrote:
> Hi Rafał,
>
> zajec5@...il.com wrote on Mon, 18 Dec 2023 23:10:20 +0100:
>
>> On 18.12.2023 15:21, Miquel Raynal wrote:
>>> Hi Rafał,
>>>
>>> zajec5@...il.com wrote on Mon, 18 Dec 2023 14:37:22 +0100:
>>>
>>>> From: Rafał Miłecki <rafal@...ecki.pl>
>>>>
>>>> This patch moves all generic (NVMEM devices independent) code from NVMEM
>>>> device driver to NVMEM layout driver. Then it adds a simple NVMEM layout
>>>> code on top of it.
>>>>
>>>> Thanks to proper layout it's possible to support U-Boot env data stored
>>>> on any kind of NVMEM device.
>>>>
>>>> For backward compatibility with old DT bindings we need to keep old
>>>> NVMEM device driver functional. To avoid code duplication a parsing
>>>> function is exported and reused in it.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
>>>> ---
>>>
>>> I have a couple of comments about the original driver which gets
>>> copy-pasted in the new layout driver, maybe you could clean these
>>> (the memory leak should be fixed before the migration so it can be
>>> backported easily, the others are just style so it can be done after, I
>>> don't mind).
>>>
>>> ...
>>>
>>>> +int u_boot_env_parse(struct device *dev, struct nvmem_device *nvmem,
>>>> + enum u_boot_env_format format)
>>>> +{
>>>> + size_t crc32_data_offset;
>>>> + size_t crc32_data_len;
>>>> + size_t crc32_offset;
>>>> + size_t data_offset;
>>>> + size_t data_len;
>>>> + size_t dev_size;
>>>> + uint32_t crc32;
>>>> + uint32_t calc;
>>>> + uint8_t *buf;
>>>> + int bytes;
>>>> + int err;
>>>> +
>>>> + dev_size = nvmem_dev_size(nvmem);
>>>> +
>>>> + buf = kcalloc(1, dev_size, GFP_KERNEL);
>>>
>>> Out of curiosity, why kcalloc(1,...) rather than kzalloc() ?
>>
>> I used kcalloc() initially as I didn't need buffer to be zeroed.
>
> I think kcalloc() initializes the memory to zero.
> https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L659
>
> If you don't need it you can switch to kmalloc() instead, I don't mind,
> but kcalloc() is meant to be used with arrays, I don't see the point of
> using kcalloc() in this case.
>
>>
>> I see that memory-allocation.rst however says:
>> > And, to be on the safe side it's best to use routines that set memory to zero, like kzalloc().
>>
>> It's probably close to zero cost to zero that buffer so it could be kzalloc().
>>
>>
>>>> + if (!buf) {
>>>> + err = -ENOMEM;
>>>> + goto err_out;
>>>
>>> We could directly return ENOMEM here I guess.
>>>
>>>> + }
>>>> +
>>>> + bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
>>>> + if (bytes < 0)
>>>> + return bytes;
>>>> + else if (bytes != dev_size)
>>>> + return -EIO;
>>>
>>> Don't we need to free buf in the above cases?
>>>
>>>> + switch (format) {
>>>> + case U_BOOT_FORMAT_SINGLE:
>>>> + crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
>>>> + crc32_data_offset = offsetof(struct u_boot_env_image_single, data);
>>>> + data_offset = offsetof(struct u_boot_env_image_single, data);
>>>> + break;
>>>> + case U_BOOT_FORMAT_REDUNDANT:
>>>> + crc32_offset = offsetof(struct u_boot_env_image_redundant, crc32);
>>>> + crc32_data_offset = offsetof(struct u_boot_env_image_redundant, data);
>>>> + data_offset = offsetof(struct u_boot_env_image_redundant, data);
>>>> + break;
>>>> + case U_BOOT_FORMAT_BROADCOM:
>>>> + crc32_offset = offsetof(struct u_boot_env_image_broadcom, crc32);
>>>> + crc32_data_offset = offsetof(struct u_boot_env_image_broadcom, data);
>>>> + data_offset = offsetof(struct u_boot_env_image_broadcom, data);
>>>> + break;
>>>> + }
>>>> + crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
>>>
>>> Looks a bit convoluted, any chances we can use intermediate variables
>>> to help decipher this?
>>>
>>>> + crc32_data_len = dev_size - crc32_data_offset;
>>>> + data_len = dev_size - data_offset;
>>>> +
>>>> + calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
>>>> + if (calc != crc32) {
>>>> + dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32);
>>>> + err = -EINVAL;
>>>> + goto err_kfree;
>>>> + }
>>>> +
>>>> + buf[dev_size - 1] = '\0';
>>>> + err = u_boot_env_parse_cells(dev, nvmem, buf, data_offset, data_len);
>>>> + if (err)
>>>> + dev_err(dev, "Failed to add cells: %d\n", err);
>>>
>>> Please drop this error message, the only reason for which the function
>>> call would fail is apparently an ENOMEM case.
>>>
>>>> +
>>>> +err_kfree:
>>>> + kfree(buf);
>>>> +err_out:
>>>> + return err;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
>>>> +
>>>> +static int u_boot_env_add_cells(struct device *dev, struct nvmem_device *nvmem)
>>>> +{
>>>> + const struct of_device_id *match;
>>>> + struct device_node *layout_np;
>>>> + enum u_boot_env_format format;
>>>> +
>>>> + layout_np = of_nvmem_layout_get_container(nvmem);
>>>> + if (!layout_np)
>>>> + return -ENOENT;
>>>> +
>>>> + match = of_match_node(u_boot_env_of_match_table, layout_np);
>>>> + if (!match)
>>>> + return -ENOENT;
>>>> +
>>>> + format = (uintptr_t)match->data;
>>>
>>> In the core there is currently an unused helper called
>>> nvmem_layout_get_match_data() which does that. I think the original
>>> intent of this function was to be used in this driver, so depending on
>>> your preference, can you please either use it or remove it?
>>
>> The problem is that nvmem_layout_get_match_data() uses:
>> layout->dev.driver
>
> I'm surprised .driver is unset. Well anyway, please either fix the core
> helper and use it or drop the core helper, because we have no user for
> it otherwise?
I believe it's because of a very minimalistic "nvmem_bus_type" bus
implementation.
From a quick look it seems that default expected FORWARD-trace is:
driver_register()
bus_add_driver()
driver_attach()
__driver_attach()
driver_probe_device()
__driver_probe_device()
really_probe()
It's really_probe() that seems to set dev->driver pointer.
>> It doesn't work with layouts driver (since refactoring?) as driver is
>> NULL. That results in NULL pointer dereference when trying to reach
>> of_match_table.
>>
>> That is why I used u_boot_env_of_match_table directly.
>>
>> If you know how to fix nvmem_layout_get_match_data() that would be
>> great. Do we need driver_register() somewhere in NVMEM core?
Powered by blists - more mailing lists