[PATCH] of: base: Handle optional argument in of_parse_phandle_with_args_map()

[PATCH] of: base: Handle optional argument in of_parse_phandle_with_args_map()

[Miquel Raynal (Schneider Electric)]

The kernel doc claims out_args is optional. In practice, the function
unconditionally dereferences it. It feels like calling this function
with a NULL out_args argument may be a valid use case (in order to know
if the lookup would work, even though we might not care about the
result), even though in practice there seem to be no actual user of that
possibility. All callers give an on-stack object to fill.

There are two ways out: either we drop the "optional" wording from the
kernel doc, or we handle the NULL case properly. Since handling this
case goes in favor of more harmonization of the semantics and since
of_parse_phandle_with_args() actually do handle that situation
correctly, let's handle it in the _map() case as well.

Use a local on-stack object in case no argument is passed. This way, we
actually go through the same logic as if there was an argument. We may
however return rather early, since the second part of the function is
dedicated at filling that argument.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260710183359.61D3A1F000E9@smtp.kernel.org/
Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
---
The number of passing unit tests is identical before/after the commit:
[   10.774120] ### dt-test ### end of unittest - 426 passed, 1 failed
---
 drivers/of/base.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..dd7dda0e0a57 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1505,15 +1505,15 @@ EXPORT_SYMBOL(__of_parse_phandle_with_args);
  * @list_name:	property name that contains a list
  * @stem_name:	stem of property names that specify phandles' arguments count
  * @index:	index of a phandle to parse out
- * @out_args:	optional pointer to output arguments structure (will be filled)
+ * @_out_args:	optional pointer to output arguments structure (will be filled)
  *
  * This function is useful to parse lists of phandles and their arguments.
- * Returns 0 on success and fills out_args, on error returns appropriate errno
+ * Returns 0 on success and fills @_out_args, on error returns appropriate errno
  * value. The difference between this function and of_parse_phandle_with_args()
  * is that this API remaps a phandle if the node the phandle points to has
  * a <@stem_name>-map property.
  *
- * Caller is responsible to call of_node_put() on the returned out_args->np
+ * Caller is responsible to call of_node_put() on the returned @_out_args->np
  * pointer.
  *
  * Example::
@@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(__of_parse_phandle_with_args);
 int of_parse_phandle_with_args_map(const struct device_node *np,
 				   const char *list_name,
 				   const char *stem_name,
-				   int index, struct of_phandle_args *out_args)
+				   int index, struct of_phandle_args *_out_args)
 {
 	char *cells_name __free(kfree) = kasprintf(GFP_KERNEL, "#%s-cells", stem_name);
 	char *map_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map", stem_name);
@@ -1554,6 +1554,8 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 	const __be32 *map, *mask, *pass;
 	static const __be32 dummy_mask[] = { [0 ... (MAX_PHANDLE_ARGS - 1)] = cpu_to_be32(~0) };
 	static const __be32 dummy_pass[] = { [0 ... (MAX_PHANDLE_ARGS - 1)] = cpu_to_be32(0) };
+	struct of_phandle_args _oa = {};
+	struct of_phandle_args *out_args = _out_args ? _out_args : &_oa;
 	__be32 initial_match_array[MAX_PHANDLE_ARGS];
 	const __be32 *match_array = initial_match_array;
 	int i, ret, map_len, match;
@@ -1585,6 +1587,8 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
 		/* Get the <list>-map property */
 		map = of_get_property(cur, map_name, &map_len);
 		if (!map) {
+			if (!_out_args)
+				of_node_put(out_args->np);
 			return 0;
 		}
 		map_len /= sizeof(u32);

---
base-commit: 7f26e010764df304602f33912a0550dcf46e72c2
change-id: 20260716-schneider-v7-2-rc1-eip-of-fix-c812599db058

Best regards,
-- 
Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

Re: [PATCH] of: base: Handle optional argument in of_parse_phandle_with_args_map()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on `list_size` causes an out-of-bounds stack write and read when `#<list>-cells` exceeds `MAX_PHANDLE_ARGS`.
--

--- Patch [1]: [PATCH] of: base: Handle optional argument in of_parse_phandle_with_args_map() ---
commit 22223dd6c28b67c741e12313ce0dc7dc5d351701
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

of: base: Handle optional argument in of_parse_phandle_with_args_map()

This commit handles the optional NULL out_args argument by providing a local
on-stack fallback object. This avoids unconditional null pointer dereferences
when the caller only wants to verify if the lookup works without keeping
the result.

Closes: https://lore.kernel.org/all/20260710183359.61D3A1F000E9@smtp.kernel.org/

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
[ ... ]
> @@ -1554,6 +1554,8 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
>  	const __be32 *map, *mask, *pass;
>  	static const __be32 dummy_mask[] = { [0 ... (MAX_PHANDLE_ARGS - 1)] = cpu_to_be32(~0) };
>  	static const __be32 dummy_pass[] = { [0 ... (MAX_PHANDLE_ARGS - 1)] = cpu_to_be32(0) };
> +	struct of_phandle_args _oa = {};
> +	struct of_phandle_args *out_args = _out_args ? _out_args : &_oa;
>  	__be32 initial_match_array[MAX_PHANDLE_ARGS];

[Severity: High]
This is a pre-existing issue, but could a malformed device tree cause an
out-of-bounds stack write to initial_match_array?

Later in of_parse_phandle_with_args_map(), the #<list>-cells property is
read directly into list_size without being bounded to MAX_PHANDLE_ARGS:

	ret = of_property_read_u32(cur, cells_name, &list_size);
	if (ret < 0)
		goto put;

	/* Precalculate the match array - this simplifies match loop */
	for (i = 0; i < list_size; i++)
		initial_match_array[i] = cpu_to_be32(out_args->args[i]);

If a device tree node defines a #<list>-cells value greater than 16, this
loop will read out of bounds from out_args->args and write out of bounds to
the initial_match_array stack buffer.

Does list_size need to be validated against MAX_PHANDLE_ARGS before being
used as a loop limit?

>  	const __be32 *match_array = initial_match_array;
>  	int i, ret, map_len, match;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-schneider-v7-2-rc1-eip-of-fix-v1-1-9b9383f936c6@bootlin.com?part=1