| Message ID | 20231124180136.390621-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: stream: fix NULL pointer dereference for multi_link | expand |
On 11/24/23 12:01, Krzysztof Kozlowski wrote: > If bus is marked as multi_link, but number of masters in the stream is > not higher than bus->hw_sync_min_links (bus->multi_link && m_rt_count >= > bus->hw_sync_min_links), bank switching should not happen. The first > part of do_bank_switch() code properly takes these conditions into > account, but second part (sdw_ml_sync_bank_switch()) relies purely on > bus->multi_link property. This is not balanced and leads to NULL > pointer dereference: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 > ... > Call trace: > wait_for_completion_timeout+0x124/0x1f0 > do_bank_switch+0x370/0x6f8 > sdw_prepare_stream+0x2d0/0x438 > qcom_snd_sdw_prepare+0xa0/0x118 > sm8450_snd_prepare+0x128/0x148 > snd_soc_link_prepare+0x5c/0xe8 > __soc_pcm_prepare+0x28/0x1ec > dpcm_be_dai_prepare+0x1e0/0x2c0 > dpcm_fe_dai_prepare+0x108/0x28c > snd_pcm_do_prepare+0x44/0x68 > snd_pcm_action_single+0x54/0xc0 > snd_pcm_action_nonatomic+0xe4/0xec > snd_pcm_prepare+0xc4/0x114 > snd_pcm_common_ioctl+0x1154/0x1cc0 > snd_pcm_ioctl+0x54/0x74 > > Fixes: ce6e74d008ff ("soundwire: Add support for multi link bank switch") > Cc: <stable@vger.kernel.org> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> LGTM, thanks for the patch. Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/stream.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index 9dc6399f206a..f9c0adc0738d 100644 > --- a/drivers/soundwire/stream.c > +++ b/drivers/soundwire/stream.c > @@ -742,14 +742,15 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) > * sdw_ml_sync_bank_switch: Multilink register bank switch > * > * @bus: SDW bus instance > + * @multi_link: whether this is a multi-link stream with hardware-based sync > * > * Caller function should free the buffers on error > */ > -static int sdw_ml_sync_bank_switch(struct sdw_bus *bus) > +static int sdw_ml_sync_bank_switch(struct sdw_bus *bus, bool multi_link) > { > unsigned long time_left; > > - if (!bus->multi_link) > + if (!multi_link) > return 0; > > /* Wait for completion of transfer */ > @@ -847,7 +848,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) > bus->bank_switch_timeout = DEFAULT_BANK_SWITCH_TIMEOUT; > > /* Check if bank switch was successful */ > - ret = sdw_ml_sync_bank_switch(bus); > + ret = sdw_ml_sync_bank_switch(bus, multi_link); > if (ret < 0) { > dev_err(bus->dev, > "multi link bank switch failed: %d\n", ret);
On Fri, 24 Nov 2023 19:01:36 +0100, Krzysztof Kozlowski wrote: > If bus is marked as multi_link, but number of masters in the stream is > not higher than bus->hw_sync_min_links (bus->multi_link && m_rt_count >= > bus->hw_sync_min_links), bank switching should not happen. The first > part of do_bank_switch() code properly takes these conditions into > account, but second part (sdw_ml_sync_bank_switch()) relies purely on > bus->multi_link property. This is not balanced and leads to NULL > pointer dereference: > > [...] Applied, thanks! [1/1] soundwire: stream: fix NULL pointer dereference for multi_link commit: e199bf52ffda8f98f129728d57244a9cd9ad5623 Best regards,
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 9dc6399f206a..f9c0adc0738d 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -742,14 +742,15 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) * sdw_ml_sync_bank_switch: Multilink register bank switch * * @bus: SDW bus instance + * @multi_link: whether this is a multi-link stream with hardware-based sync * * Caller function should free the buffers on error */ -static int sdw_ml_sync_bank_switch(struct sdw_bus *bus) +static int sdw_ml_sync_bank_switch(struct sdw_bus *bus, bool multi_link) { unsigned long time_left; - if (!bus->multi_link) + if (!multi_link) return 0; /* Wait for completion of transfer */ @@ -847,7 +848,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) bus->bank_switch_timeout = DEFAULT_BANK_SWITCH_TIMEOUT; /* Check if bank switch was successful */ - ret = sdw_ml_sync_bank_switch(bus); + ret = sdw_ml_sync_bank_switch(bus, multi_link); if (ret < 0) { dev_err(bus->dev, "multi link bank switch failed: %d\n", ret);
If bus is marked as multi_link, but number of masters in the stream is not higher than bus->hw_sync_min_links (bus->multi_link && m_rt_count >= bus->hw_sync_min_links), bank switching should not happen. The first part of do_bank_switch() code properly takes these conditions into account, but second part (sdw_ml_sync_bank_switch()) relies purely on bus->multi_link property. This is not balanced and leads to NULL pointer dereference: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 ... Call trace: wait_for_completion_timeout+0x124/0x1f0 do_bank_switch+0x370/0x6f8 sdw_prepare_stream+0x2d0/0x438 qcom_snd_sdw_prepare+0xa0/0x118 sm8450_snd_prepare+0x128/0x148 snd_soc_link_prepare+0x5c/0xe8 __soc_pcm_prepare+0x28/0x1ec dpcm_be_dai_prepare+0x1e0/0x2c0 dpcm_fe_dai_prepare+0x108/0x28c snd_pcm_do_prepare+0x44/0x68 snd_pcm_action_single+0x54/0xc0 snd_pcm_action_nonatomic+0xe4/0xec snd_pcm_prepare+0xc4/0x114 snd_pcm_common_ioctl+0x1154/0x1cc0 snd_pcm_ioctl+0x54/0x74 Fixes: ce6e74d008ff ("soundwire: Add support for multi link bank switch") Cc: <stable@vger.kernel.org> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/soundwire/stream.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)