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

[sashiko-bot@kernel.org]

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