Re: [PATCH v4 05/11] firewall: introduce stm32_firewall framework

[Simon Horman]

On Fri, Aug 11, 2023 at 12:07:25PM +0200, Gatien Chevallier wrote:

...

> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c
> new file mode 100644
> index 000000000000..900f3b052a66
> --- /dev/null
> +++ b/drivers/bus/stm32_firewall.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/bus/stm32_firewall_device.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +
> +#include "stm32_firewall.h"
> +
> +/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall ID */
> +#define STM32_FIREWALL_MAX_ARGS		(STM32_FIREWALL_MAX_EXTRA_ARGS + 1)
> +
> +static LIST_HEAD(firewall_controller_list);
> +static DEFINE_MUTEX(firewall_controller_list_lock);
> +
> +/* Firewall device API */
> +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall,
> +				unsigned int nb_firewall)
> +{
> +	struct stm32_firewall_controller *ctrl;
> +	struct of_phandle_iterator it;
> +	unsigned int i, j = 0;
> +	int err;
> +
> +	if (!firewall || !nb_firewall)
> +		return -EINVAL;
> +
> +	/* Parse property with phandle parsed out */
> +	of_for_each_phandle(&it, err, np, "feature-domains", "#feature-domain-cells", 0) {
> +		struct of_phandle_args provider_args;
> +		struct device_node *provider = it.node;
> +		const char *fw_entry;
> +		bool match = false;
> +
> +		if (err) {
> +			pr_err("Unable to get feature-domains property for node %s\n, err: %d",
> +			       np->full_name, err);
> +			of_node_put(provider);
> +			return err;
> +		}
> +
> +		if (j > nb_firewall) {
> +			pr_err("Too many firewall controllers");
> +			of_node_put(provider);
> +			return -EINVAL;
> +		}
> +
> +		provider_args.args_count = of_phandle_iterator_args(&it, provider_args.args,
> +								    STM32_FIREWALL_MAX_ARGS);
> +
> +		/* Check if the parsed phandle corresponds to a registered firewall controller */
> +		mutex_lock(&firewall_controller_list_lock);
> +		list_for_each_entry(ctrl, &firewall_controller_list, entry) {
> +			if (ctrl->dev->of_node->phandle == it.phandle) {
> +				match = true;
> +				firewall[j].firewall_ctrl = ctrl;
> +				break;
> +			}
> +		}
> +		mutex_unlock(&firewall_controller_list_lock);
> +
> +		if (!match) {
> +			firewall[j].firewall_ctrl = NULL;
> +			pr_err("No firewall controller registered for %s\n", np->full_name);
> +			of_node_put(provider);
> +			return -ENODEV;
> +		}
> +
> +		err = of_property_read_string_index(np, "feature-domain-names", j, &fw_entry);
> +		if (err == 0)
> +			firewall[j].entry = fw_entry;
> +
> +		/* Handle the case when there are no arguments given along with the phandle */
> +		if (provider_args.args_count < 0 ||
> +		    provider_args.args_count > STM32_FIREWALL_MAX_ARGS) {
> +			of_node_put(provider);
> +			return -EINVAL;
> +		} else if (provider_args.args_count == 0) {
> +			firewall[j].extra_args_size = 0;
> +			firewall[j].firewall_id = U32_MAX;
> +			j++;
> +			continue;
> +		}
> +
> +		/* The firewall ID is always the first argument */
> +		firewall[j].firewall_id = provider_args.args[0];
> +
> +		/* Extra args start at the third argument */
> +		for (i = 0; i < provider_args.args_count; i++)
> +			firewall[j].extra_args[i] = provider_args.args[i + 1];

Hi Gatien,

Above it is checked that the maximum value of provider_args.args_count is
STM32_FIREWALL_MAX_ARGS.
So here the maximum value of i is STM32_FIREWALL_MAX_ARGS - 1.

STM32_FIREWALL_MAX_ARGS is defined as STM32_FIREWALL_MAX_EXTRA_ARGS + 1
And STM32_FIREWALL_MAX_EXTRA_ARGS is defined as 5.
So the maximum value of i is (5 + 1 - 1) = 5.

firewall[j] is of type struct stm32_firewall.
And its args field has STM32_FIREWALL_MAX_EXTRA_ARGS (5) elements.
Thus the maximum valid index is (5 - 1) = 4.

But the line above may access index 5.

Flagged by Smatch.

> +
> +		/* Remove the firewall ID arg that is not an extra argument */
> +		firewall[j].extra_args_size = provider_args.args_count - 1;
> +
> +		j++;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall);

...

> diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h
> new file mode 100644
> index 000000000000..7b4450a8ec15
> --- /dev/null
> +++ b/include/linux/bus/stm32_firewall_device.h
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
> + */
> +
> +#ifndef STM32_FIREWALL_DEVICE_H
> +#define STM32_FIREWALL_DEVICE_H
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#define STM32_FIREWALL_MAX_EXTRA_ARGS		5
> +
> +/* Opaque reference to stm32_firewall_controller */
> +struct stm32_firewall_controller;
> +
> +/**
> + * struct stm32_firewall - Information on a device's firewall. Each device can have more than one
> + *			   firewall.
> + *
> + * @firewall_ctrl:		Pointer referencing a firewall controller of the device. It is
> + *				opaque so a device cannot manipulate the controller's ops or access
> + *				the controller's data
> + * @extra_args:			Extra arguments that are implementation dependent
> + * @entry:			Name of the firewall entry
> + * @extra_args_size:		Number of extra arguments
> + * @firewall_id:		Firewall ID associated the device for this firewall controller
> + */
> +struct stm32_firewall {
> +	struct stm32_firewall_controller *firewall_ctrl;
> +	u32 extra_args[STM32_FIREWALL_MAX_EXTRA_ARGS];
> +	const char *entry;
> +	size_t extra_args_size;
> +	u32 firewall_id;
> +};

...