Re: [PATCH 2/2] nvmem: layouts: add U-Boot env layout

[John Thomson]

On Mon, 15 Jul 2024, at 13:54, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> diff --git a/drivers/nvmem/layouts/u-boot-env.c 
> b/drivers/nvmem/layouts/u-boot-env.c
> new file mode 100644
> index 000000000000..5217dc4a52f8
> --- /dev/null
> +++ b/drivers/nvmem/layouts/u-boot-env.c
> @@ -0,0 +1,203 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 - 2023 Rafał Miłecki <rafal@xxxxxxxxxx>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/etherdevice.h>
> +#include <linux/export.h>
> +#include <linux/if_ether.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include "u-boot-env.h"
> +
> +struct u_boot_env_image_single {
> +	__le32 crc32;
> +	uint8_t data[];
> +} __packed;
> +
> +struct u_boot_env_image_redundant {
> +	__le32 crc32;
> +	u8 mark;
> +	uint8_t data[];
> +} __packed;
> +
> +struct u_boot_env_image_broadcom {
> +	__le32 magic;
> +	__le32 len;
> +	__le32 crc32;
> +	DECLARE_FLEX_ARRAY(uint8_t, data);
> +} __packed;
> +
> +static int u_boot_env_read_post_process_ethaddr(void *context, const 
> char *id, int index,
> +						unsigned int offset, void *buf, size_t bytes)
> +{
> +	u8 mac[ETH_ALEN];
> +
> +	if (bytes != 3 * ETH_ALEN - 1)
> +		return -EINVAL;
> +
> +	if (!mac_pton(buf, mac))
> +		return -EINVAL;
> +
> +	if (index)
> +		eth_addr_add(mac, index);
> +
> +	ether_addr_copy(buf, mac);
> +
> +	return 0;
> +}
> +
> +static int u_boot_env_parse_cells(struct device *dev, struct 
> nvmem_device *nvmem, uint8_t *buf,
> +				  size_t data_offset, size_t data_len)
> +{
> +	char *data = buf + data_offset;
> +	char *var, *value, *eq;
> +
> +	for (var = data;
> +	     var < data + data_len && *var;
> +	     var = value + strlen(value) + 1) {
> +		struct nvmem_cell_info info = {};
> +
> +		eq = strchr(var, '=');
> +		if (!eq)
> +			break;
> +		*eq = '\0';
> +		value = eq + 1;
> +
> +		info.name = devm_kstrdup(dev, var, GFP_KERNEL);
> +		if (!info.name)
> +			return -ENOMEM;
> +		info.offset = data_offset + value - data;
> +		info.bytes = strlen(value);
> +		info.np = of_get_child_by_name(dev->of_node, info.name);
> +		if (!strcmp(var, "ethaddr")) {
> +			info.raw_len = strlen(value);
> +			info.bytes = ETH_ALEN;
> +			info.read_post_process = u_boot_env_read_post_process_ethaddr;
> +		}
> +
> +		nvmem_add_one_cell(nvmem, &info);
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +	__le32 *crc32_addr;
> +	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 = kzalloc(dev_size, GFP_KERNEL);
> +	if (!buf) {
> +		err = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	bytes = nvmem_device_read(nvmem, 0, dev_size, buf);
> +	if (bytes < 0) {
> +		err = bytes;
> +		goto err_kfree;
> +	} else if (bytes != dev_size) {
> +		err = -EIO;
> +		goto err_kfree;
> +	}
> +
> +	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;
> +	}

Could we error somewhere if bytes or dev_size is < crc32_data_offset
or alternatively if bytes is zero (above, beside bytes != dev_size)?
Detailed here: https://lore.kernel.org/lkml/20240612031510.14414-1-git@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
mtd partitioning can deliver a zero length partition (if misconfigured: Example u-boot partition starts beyond hardware NOR length).

> +	crc32_addr = (__le32 *)(buf + crc32_offset);
> +	crc32 = le32_to_cpu(*crc32_addr);
> +	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);
> +
> +err_kfree:
> +	kfree(buf);
> +err_out:
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(u_boot_env_parse);
> +
> +static int u_boot_env_add_cells(struct nvmem_layout *layout)
> +{
> +	struct device *dev = &layout->dev;
> +	enum u_boot_env_format format;
> +
> +	format = (uintptr_t)device_get_match_data(dev);
> +
> +	return u_boot_env_parse(dev, layout->nvmem, format);
> +}
> +
> +static int u_boot_env_probe(struct nvmem_layout *layout)
> +{
> +	layout->add_cells = u_boot_env_add_cells;
> +
> +	return nvmem_layout_register(layout);
> +}
> +
> +static void u_boot_env_remove(struct nvmem_layout *layout)
> +{
> +	nvmem_layout_unregister(layout);
> +}
> +
> +static const struct of_device_id u_boot_env_of_match_table[] = {
> +	{ .compatible = "u-boot,env", .data = (void *)U_BOOT_FORMAT_SINGLE, },
> +	{ .compatible = "u-boot,env-redundant-bool", .data = (void 
> *)U_BOOT_FORMAT_REDUNDANT, },
> +	{ .compatible = "u-boot,env-redundant-count", .data = (void 
> *)U_BOOT_FORMAT_REDUNDANT, },
> +	{ .compatible = "brcm,env", .data = (void *)U_BOOT_FORMAT_BROADCOM, },
> +	{},
> +};
> +
> +static struct nvmem_layout_driver u_boot_env_layout = {
> +	.driver = {
> +		.name = "u-boot-env-layout",
> +		.of_match_table = u_boot_env_of_match_table,
> +	},
> +	.probe = u_boot_env_probe,
> +	.remove = u_boot_env_remove,
> +};
> +module_nvmem_layout_driver(u_boot_env_layout);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table);


-- 
  John Thomson