[PATCH] soundwire: mipi_disco: Put fwnode on property array read errors

[PATCH] soundwire: mipi_disco: Put fwnode on property array read errors

[Xiangwu Lai]

sdw_master_read_prop() and sdw_slave_read_dpn() take a fwnode
reference with device_get_named_child_node() and already release it
on the -ENOMEM and success paths.  However, the error paths added for
the property array reads jump straight out without dropping the
reference, leaking it on every failed read.

Release the reference on those error paths as well, matching the
existing -ENOMEM handling in the same functions.

Fixes: a489afc105ed ("soundwire: mipi_disco: add error handling for property array read")
Signed-off-by: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>
---
 drivers/soundwire/mipi_disco.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 259ad98b5..65f8a41e6 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -96,8 +96,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 		ret = fwnode_property_read_u32_array(link,
 				"mipi-sdw-clock-frequencies-supported",
 				prop->clk_freq, prop->num_clk_freq);
-		if (ret < 0)
+		if (ret < 0) {
+			fwnode_handle_put(link);
 			return ret;
+		}
 	}
 
 	/*
@@ -132,8 +134,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
 					       scales_prop,
 					       prop->clk_gears,
 					       prop->num_clk_gears);
-		if (ret < 0)
+		if (ret < 0) {
+			fwnode_handle_put(link);
 			return ret;
+		}
 	}
 
 	fwnode_property_read_u32(link, "mipi-sdw-default-frame-rate",
@@ -262,8 +266,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 			ret = fwnode_property_read_u32_array(node,
 					"mipi-sdw-port-wordlength-configs",
 					dpn[i].words, dpn[i].num_words);
-			if (ret < 0)
+			if (ret < 0) {
+				fwnode_handle_put(node);
 				return ret;
+			}
 		}
 
 		fwnode_property_read_u32(node, "mipi-sdw-data-port-type",
@@ -305,8 +311,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 			ret = fwnode_property_read_u32_array(node,
 					"mipi-sdw-channel-number-list",
 					dpn[i].channels, dpn[i].num_channels);
-			if (ret < 0)
+			if (ret < 0) {
+				fwnode_handle_put(node);
 				return ret;
+			}
 		}
 
 		nval = fwnode_property_count_u32(node, "mipi-sdw-channel-combination-list");
@@ -325,8 +333,10 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 					"mipi-sdw-channel-combination-list",
 					dpn[i].ch_combinations,
 					dpn[i].num_ch_combinations);
-			if (ret < 0)
+			if (ret < 0) {
+				fwnode_handle_put(node);
 				return ret;
+			}
 		}
 
 		fwnode_property_read_u32(node,
@@ -347,14 +357,18 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
 			dpn[i].lane_list = devm_kcalloc(&slave->dev,
 							dpn[i].num_lanes, sizeof(*dpn[i].lane_list),
 							GFP_KERNEL);
-			if (!dpn[i].lane_list)
+			if (!dpn[i].lane_list) {
+				fwnode_handle_put(node);
 				return -ENOMEM;
+			}
 
 			ret = fwnode_property_read_u32_array(node,
 						       "mipi-sdw-lane-list",
 						       dpn[i].lane_list, dpn[i].num_lanes);
-			if (ret < 0)
+			if (ret < 0) {
+				fwnode_handle_put(node);
 				return ret;
+			}
 		}
 
 		fwnode_handle_put(node);
-- 
2.50.1.windows.1

Re: [PATCH] soundwire: mipi_disco: Put fwnode on property array read errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] soundwire: mipi_disco: inaccurate Fixes trailer and missing tag
  for lane-list
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] soundwire: master: buffer overflow in clock sysfs show
  callbacks [1]
--

commit 64b469cb0e928d1152da84b0b4992092509cd6b6
Author: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>

> soundwire: mipi_disco: Put fwnode on property array read errors
>
> sdw_master_read_prop() and sdw_slave_read_dpn() take a fwnode
> reference with device_get_named_child_node() and already release it
> on the -ENOMEM and success paths.  However, the error paths added for
> the property array reads jump straight out without dropping the
> reference, leaking it on every failed read.
>
> Release the reference on those error paths as well, matching the
> existing -ENOMEM handling in the same functions.
>
> Fixes: a489afc105ed ("soundwire: mipi_disco: add error handling for property array read")
> Signed-off-by: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>

[Severity: Low]
The Fixes: tag has a slight subject mismatch ("mipi_disco" vs
"mipi-disco"); the canonical tag is:

  Fixes: a489afc105ed ("soundwire: mipi-disco: add error handling for property array read")

In addition, the mipi-sdw-lane-list handling in sdw_slave_read_dpn(),
where this patch fixes both the -ENOMEM path (!dpn[i].lane_list) and the
fwnode_property_read_u32_array() error path, was introduced by
71b405b18444 ("soundwire: mipi-disco: add support for DP0/DPn 'lane-list'
property").  Should the commit message note that -ENOMEM fix and also
include:

  Fixes: 71b405b18444 ("soundwire: mipi-disco: add support for DP0/DPn 'lane-list' property")

> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index 259ad98b5b7a9..65f8a41e6cd90 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -96,8 +96,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
>  		ret = fwnode_property_read_u32_array(link,
>  				"mipi-sdw-clock-frequencies-supported",
>  				prop->clk_freq, prop->num_clk_freq);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			fwnode_handle_put(link);
>  			return ret;
> +		}
>  	}
>  
>  	/*
> @@ -132,8 +134,10 @@ int sdw_master_read_prop(struct sdw_bus *bus)
>  					       scales_prop,
>  					       prop->clk_gears,
>  					       prop->num_clk_gears);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			fwnode_handle_put(link);
>  			return ret;
> +		}
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can prop->num_clk_freq
and prop->num_clk_gears cause a buffer overflow [1] in
clock_frequencies_show() and clock_gears_show()?

sdw_master_read_prop() sets prop->num_clk_freq and prop->num_clk_gears
directly from fwnode_property_count_u32() without an upper bound, and the
sysfs show callbacks format each entry into the PAGE_SIZE buffer with
unbounded sprintf() calls:

drivers/soundwire/master.c:clock_frequencies_show() {
    ...
	for (i = 0; i < md->bus->prop.num_clk_freq; i++)
		size += sprintf(buf + size, "%8d ",
				md->bus->prop.clk_freq[i]);
	size += sprintf(buf + size, "\n");
    ...
}

If firmware provides 456 or more clock frequency or clock gear entries,
reading the sysfs attribute writes past the end of the 4096-byte buffer
and corrupts kernel heap memory.  Using sysfs_emit_at() in
clock_frequencies_show() and clock_gears_show() would prevent the
overflow.

[1] https://sashiko.dev/#/bug/linux-6aa03701-2826-4725-b0af-33189f2d3db9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112021827.1835-1-laixiangwu@cmss.chinamobile.com?part=1

Re: [PATCH] soundwire: mipi_disco: Put fwnode on property array read errors

[Xiangwu Lai]

Hi,

thanks for the review.

Both Fixes tag corrections are right: the subject uses "mipi-disco",
and the lane-list error paths (including the -ENOMEM one, which was
missing the put unlike its siblings) were introduced by 71b405b18444.
v2 carries both Fixes tags and mentions the -ENOMEM fix explicitly.

The clock sysfs buffer overflow is also real: prop->num_clk_freq and
prop->num_clk_gears are taken from firmware-supplied properties
without an upper bound, and the show callbacks use unbounded
sprintf().  It is already being addressed by an in-flight patch from
Weigang He, so I did not duplicate it:

  https://lore.kernel.org/all/20260926042935.2284011-1-geoffreyhe2@gmail.com/

v2 follows shortly.

Best regards,
Xiangwu Lai