Re: [PATCH v15 2/3] of: Factor arguments passed to of_map_id() into a struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional dereference of `msi_node` in `of_check_msi_parent` when it can be NULL.
--

commit d17879206927f3ee695759bc3559fd04a6a551ab
Author: Charan Teja Kalla <charan.kalla@oss.qualcomm.com>
Subject: of: Factor arguments passed to of_map_id() into a struct

This patch updates of_map_id() to take a struct of_phandle_args instead of
passing the target device node and translated IDs separately. It explicitly
separates the input filter node from the output arguments and transfers
ownership of the matched node reference to the caller.

> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index e37c1b3f87362..e63a43be6c4a3 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -796,14 +796,15 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
>  /**
>   * of_msi_xlate - map a MSI ID and find relevant MSI controller node
>   * @dev: device for which the mapping is to be done.
> - * @msi_np: Pointer to target MSI controller node
> + * @msi_np: Pointer to target MSI controller node, or NULL if the caller
> + *           only needs the translated ID without receiving the controller node.
> + *           If non-NULL and pointing to a non-NULL node, only entries targeting
> + *           that node will be matched. If non-NULL and pointing to NULL, it will
> + *           receive the first matching target node with a reference held.
>   * @id_in: Device ID.
>   *
>   * Walk up the device hierarchy looking for devices with a "msi-map"
>   * or "msi-parent" property. If found, apply the mapping to @id_in.
> - * If @msi_np points to a non-NULL device node pointer, only entries targeting
> - * that node will be matched; if it points to a NULL value, it will receive the
> - * device node of the first matching target phandle, with a reference held.
>   *
>   * Returns: The mapped MSI id.
>   */
> @@ -817,8 +818,21 @@ u32 of_msi_xlate(struct device *dev, struct device_node **msi_np, u32 id_in)
>  	 * "msi-map" or an "msi-parent" property.
>  	 */
>  	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> -		if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &id_out))
> +		struct of_phandle_args msi_spec = {};
> +
> +		if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
> +			if (msi_spec.np) {
> +				/* msi-map matched: use the translated ID and target node */
> +				if (msi_spec.args_count > 0)
> +					id_out = msi_spec.args[0];
> +				if (msi_np && !*msi_np)
> +					*msi_np = of_node_get(msi_spec.np);
> +				of_node_put(msi_spec.np);
> +			}
> +			/* msi-map present but no match → stop walking */
>  			break;
> +		}
> +		/* -ENODEV: msi-map absent → check for msi-parent */
>  		if (!of_check_msi_parent(parent_dev->of_node, msi_np))

[Severity: High]
This is a pre-existing issue, but does of_check_msi_parent() handle a NULL
msi_np pointer correctly here?

The documentation added above explicitly states that msi_np can be NULL if
the caller only needs the translated ID without receiving the controller node.
However, if msi-map is absent, execution falls through to
of_check_msi_parent() with that potentially NULL pointer.

Looking at drivers/of/irq.c:of_check_msi_parent(), if the msi-parent
property is found, it evaluates:

	if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
		ret = -EINVAL;

	if (!ret) {
		/* Return with a node reference held */
		*msi_node = msi_spec.np;
		return 0;
	}

If msi_np is NULL, wouldn't dereferencing it via *msi_node trigger a NULL
pointer dereference? Should this be fixed to safely handle a NULL pointer?

>  			break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-parse_iommu_cells-v15-0-b5f99ad4e7e8@oss.qualcomm.com?part=2