| 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: <29d2f463-90c2-4c96-8828-96806679db8e@linaro.org>
Date: Fri, 8 Dec 2023 11:15:24 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Rafał Miłecki <zajec5@...il.com>
Cc: linux-mtd@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Rafał Miłecki <rafal@...ecki.pl>,
Arınç ÜNAL <arinc.unal@...nc9.com>,
Florian Fainelli <florian.fainelli@...adcom.com>,
Scott Branden <scott.branden@...adcom.com>
Subject: Re: [PATCH V3] nvmem: brcm_nvram: store a copy of NVRAM content
Thank you for the patch,
On 02/11/2023 06:28, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@...ecki.pl>
>
> This driver uses MMIO access for reading NVRAM from a flash device.
> Underneath there is a flash controller that reads data and provides
> mapping window.
>
> Using MMIO interface affects controller configuration and may break real
> controller driver. It was reported by multiple users of devices with
> NVRAM stored on NAND.
>
> Modify driver to read & cache NVRAM content during init and use that
> copy to provide NVMEM data when requested. On NAND flashes due to their
> alignment NVRAM partitions can be quite big (1 MiB and more) while
> actual NVRAM content stays quite small (usually 16 to 32 KiB). To avoid
> allocating so much memory check for actual data length.
>
> Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=K0EoWXuXcSMkJO1Vy_5vmVqA@mail.gmail.com/
> Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing Broadcom's NVRAM")
Any reason not to add
Cc: <Stable@...r.kernel.org>
?
--srini
> Cc: Arınç ÜNAL <arinc.unal@...nc9.com>
> Cc: Florian Fainelli <florian.fainelli@...adcom.com>
> Cc: Scott Branden <scott.branden@...adcom.com>
> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
> Acked-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> ---
> V2: Minialize amount of allocated memory (check for actual data length)
> V3: Document missed fields in struct brcm_nvram
>
> drivers/nvmem/brcm_nvram.c | 134 ++++++++++++++++++++++++++-----------
> 1 file changed, 94 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/nvmem/brcm_nvram.c b/drivers/nvmem/brcm_nvram.c
> index 9737104f3b76..5cdf339cfbec 100644
> --- a/drivers/nvmem/brcm_nvram.c
> +++ b/drivers/nvmem/brcm_nvram.c
> @@ -17,9 +17,23 @@
>
> #define NVRAM_MAGIC "FLSH"
>
> +/**
> + * struct brcm_nvram - driver state internal struct
> + *
> + * @dev: NVMEM device pointer
> + * @nvmem_size: Size of the whole space available for NVRAM
> + * @data: NVRAM data copy stored to avoid poking underlaying flash controller
> + * @data_len: NVRAM data size
> + * @padding_byte: Padding value used to fill remaining space
> + * @cells: Array of discovered NVMEM cells
> + * @ncells: Number of elements in cells
> + */
> struct brcm_nvram {
> struct device *dev;
> - void __iomem *base;
> + size_t nvmem_size;
> + uint8_t *data;
> + size_t data_len;
> + uint8_t padding_byte;
> struct nvmem_cell_info *cells;
> int ncells;
> };
> @@ -36,10 +50,47 @@ static int brcm_nvram_read(void *context, unsigned int offset, void *val,
> size_t bytes)
> {
> struct brcm_nvram *priv = context;
> - u8 *dst = val;
> + size_t to_copy;
> +
> + if (offset + bytes > priv->data_len)
> + to_copy = max_t(ssize_t, (ssize_t)priv->data_len - offset, 0);
> + else
> + to_copy = bytes;
> +
> + memcpy(val, priv->data + offset, to_copy);
> +
> + memset((uint8_t *)val + to_copy, priv->padding_byte, bytes - to_copy);
> +
> + return 0;
> +}
> +
> +static int brcm_nvram_copy_data(struct brcm_nvram *priv, struct platform_device *pdev)
> +{
> + struct resource *res;
> + void __iomem *base;
> +
> + base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + priv->nvmem_size = resource_size(res);
> +
> + priv->padding_byte = readb(base + priv->nvmem_size - 1);
> + for (priv->data_len = priv->nvmem_size;
> + priv->data_len;
> + priv->data_len--) {
> + if (readb(base + priv->data_len - 1) != priv->padding_byte)
> + break;
> + }
> + WARN(priv->data_len > SZ_128K, "Unexpected (big) NVRAM size: %zu B\n", priv->data_len);
> +
> + priv->data = devm_kzalloc(priv->dev, priv->data_len, GFP_KERNEL);
> + if (!priv->data)
> + return -ENOMEM;
> +
> + memcpy_fromio(priv->data, base, priv->data_len);
>
> - while (bytes--)
> - *dst++ = readb(priv->base + offset++);
> + bcm47xx_nvram_init_from_iomem(base, priv->data_len);
>
> return 0;
> }
> @@ -67,8 +118,13 @@ static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
> size_t len)
> {
> struct device *dev = priv->dev;
> - char *var, *value, *eq;
> + char *var, *value;
> + uint8_t tmp;
> int idx;
> + int err = 0;
> +
> + tmp = priv->data[len - 1];
> + priv->data[len - 1] = '\0';
>
> priv->ncells = 0;
> for (var = data + sizeof(struct brcm_nvram_header);
> @@ -78,67 +134,68 @@ static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
> }
>
> priv->cells = devm_kcalloc(dev, priv->ncells, sizeof(*priv->cells), GFP_KERNEL);
> - if (!priv->cells)
> - return -ENOMEM;
> + if (!priv->cells) {
> + err = -ENOMEM;
> + goto out;
> + }
>
> for (var = data + sizeof(struct brcm_nvram_header), idx = 0;
> var < (char *)data + len && *var;
> var = value + strlen(value) + 1, idx++) {
> + char *eq, *name;
> +
> eq = strchr(var, '=');
> if (!eq)
> break;
> *eq = '\0';
> + name = devm_kstrdup(dev, var, GFP_KERNEL);
> + *eq = '=';
> + if (!name) {
> + err = -ENOMEM;
> + goto out;
> + }
> value = eq + 1;
>
> - priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
> - if (!priv->cells[idx].name)
> - return -ENOMEM;
> + priv->cells[idx].name = name;
> priv->cells[idx].offset = value - (char *)data;
> priv->cells[idx].bytes = strlen(value);
> priv->cells[idx].np = of_get_child_by_name(dev->of_node, priv->cells[idx].name);
> - if (!strcmp(var, "et0macaddr") ||
> - !strcmp(var, "et1macaddr") ||
> - !strcmp(var, "et2macaddr")) {
> + if (!strcmp(name, "et0macaddr") ||
> + !strcmp(name, "et1macaddr") ||
> + !strcmp(name, "et2macaddr")) {
> priv->cells[idx].raw_len = strlen(value);
> priv->cells[idx].bytes = ETH_ALEN;
> priv->cells[idx].read_post_process = brcm_nvram_read_post_process_macaddr;
> }
> }
>
> - return 0;
> +out:
> + priv->data[len - 1] = tmp;
> + return err;
> }
>
> static int brcm_nvram_parse(struct brcm_nvram *priv)
> {
> + struct brcm_nvram_header *header = (struct brcm_nvram_header *)priv->data;
> struct device *dev = priv->dev;
> - struct brcm_nvram_header header;
> - uint8_t *data;
> size_t len;
> int err;
>
> - memcpy_fromio(&header, priv->base, sizeof(header));
> -
> - if (memcmp(header.magic, NVRAM_MAGIC, 4)) {
> + if (memcmp(header->magic, NVRAM_MAGIC, 4)) {
> dev_err(dev, "Invalid NVRAM magic\n");
> return -EINVAL;
> }
>
> - len = le32_to_cpu(header.len);
> -
> - data = kzalloc(len, GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> - memcpy_fromio(data, priv->base, len);
> - data[len - 1] = '\0';
> -
> - err = brcm_nvram_add_cells(priv, data, len);
> - if (err) {
> - dev_err(dev, "Failed to add cells: %d\n", err);
> - return err;
> + len = le32_to_cpu(header->len);
> + if (len > priv->nvmem_size) {
> + dev_err(dev, "NVRAM length (%zd) exceeds mapped size (%zd)\n", len,
> + priv->nvmem_size);
> + return -EINVAL;
> }
>
> - kfree(data);
> + err = brcm_nvram_add_cells(priv, priv->data, len);
> + if (err)
> + dev_err(dev, "Failed to add cells: %d\n", err);
>
> return 0;
> }
> @@ -150,7 +207,6 @@ static int brcm_nvram_probe(struct platform_device *pdev)
> .reg_read = brcm_nvram_read,
> };
> struct device *dev = &pdev->dev;
> - struct resource *res;
> struct brcm_nvram *priv;
> int err;
>
> @@ -159,21 +215,19 @@ static int brcm_nvram_probe(struct platform_device *pdev)
> return -ENOMEM;
> priv->dev = dev;
>
> - priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> - if (IS_ERR(priv->base))
> - return PTR_ERR(priv->base);
> + err = brcm_nvram_copy_data(priv, pdev);
> + if (err)
> + return err;
>
> err = brcm_nvram_parse(priv);
> if (err)
> return err;
>
> - bcm47xx_nvram_init_from_iomem(priv->base, resource_size(res));
> -
> config.dev = dev;
> config.cells = priv->cells;
> config.ncells = priv->ncells;
> config.priv = priv;
> - config.size = resource_size(res);
> + config.size = priv->nvmem_size;
>
> return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> }
Powered by blists - more mailing lists