[v2] ASoC: intel: avs: Fix potential memory leak in avs_pci_probe()

[PATCH v2] ASoC: intel: avs: Fix potential memory leak in avs_pci_probe()

Posted by Abdun Nihaal 2 months, 3 weeks ago

The link resources allocated in snd_hdac_ext_bus_get_ml_capabilities()
are not freed on subsequent error paths in avs_pci_probe().

Fixes: 1affc44ea5dd ("ASoC: Intel: avs: PCI driver implementation")
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---

v1->v2:
- Shorten commit message 
- Handle the case when some of the links are allocated, as pointed out
  by Cezary Rojewski

Link to V1:
https://lore.kernel.org/all/20251113104121.79484-1-nihaal@cse.iitm.ac.in/T/#u

 sound/soc/intel/avs/core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 6e0e65584c7f..f0d77f3f3a28 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -473,8 +473,13 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	}
 
 	snd_hdac_bus_parse_capabilities(bus);
-	if (bus->mlcap)
-		snd_hdac_ext_bus_get_ml_capabilities(bus);
+	if (bus->mlcap) {
+		ret = snd_hdac_ext_bus_get_ml_capabilities(bus);
+		if (ret) {
+			dev_err(dev, "failed to get multilink capabilities: %d\n", ret);
+			goto err_ml_capabilities;
+		}
+	}
 
 	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
 		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
@@ -483,7 +488,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	ret = avs_hdac_bus_init_streams(bus);
 	if (ret < 0) {
 		dev_err(dev, "failed to init streams: %d\n", ret);
-		goto err_init_streams;
+		goto err_ml_capabilities;
 	}
 
 	ret = avs_hdac_acquire_irq(adev);
@@ -515,7 +520,8 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 err_acquire_irq:
 	snd_hdac_bus_free_stream_pages(bus);
 	snd_hdac_ext_stream_free_all(bus);
-err_init_streams:
+err_ml_capabilities:
+	snd_hdac_ext_link_free_all(bus);
 	iounmap(adev->dsp_ba);
 err_remap_bar4:
 	iounmap(bus->remap_addr);
-- 
2.43.0

Re: [PATCH v2] ASoC: intel: avs: Fix potential memory leak in avs_pci_probe()

Posted by Cezary Rojewski 2 months, 3 weeks ago

On 2025-11-13 1:04 PM, Abdun Nihaal wrote:
> The link resources allocated in snd_hdac_ext_bus_get_ml_capabilities()
> are not freed on subsequent error paths in avs_pci_probe().
> 
> Fixes: 1affc44ea5dd ("ASoC: Intel: avs: PCI driver implementation")
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

> ---
> 
> v1->v2:
> - Shorten commit message
> - Handle the case when some of the links are allocated, as pointed out
>    by Cezary Rojewski
> 
> Link to V1:
> https://lore.kernel.org/all/20251113104121.79484-1-nihaal@cse.iitm.ac.in/T/#u
> 
>   sound/soc/intel/avs/core.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> index 6e0e65584c7f..f0d77f3f3a28 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c
> @@ -473,8 +473,13 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   	}
>   
>   	snd_hdac_bus_parse_capabilities(bus);
> -	if (bus->mlcap)
> -		snd_hdac_ext_bus_get_ml_capabilities(bus);
> +	if (bus->mlcap) {
> +		ret = snd_hdac_ext_bus_get_ml_capabilities(bus);

After giving this a second thought, I believe 
snd_hdac_ext_bus_get_ml_capabilities() is the offender here - the 
function should have freed whatever its already allocated before 
returning an error, not count on the caller to free the resources 
instead. In other words, the fix should update the callee too.

However, one may say that it's a separate issue. I'm fine with existing 
patch landing as-is. Can prepare separate a change that covers problem 
mentioned by me above. The cons is: additional 1-2 LOC traffic for the 
avs-driver code.

I leave the decision to Mark, I'm OK with both approaches.

> +		if (ret) {
> +			dev_err(dev, "failed to get multilink capabilities: %d\n", ret);
> +			goto err_ml_capabilities;
> +		}
> +	}
>   
>   	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
>   		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> @@ -483,7 +488,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   	ret = avs_hdac_bus_init_streams(bus);
>   	if (ret < 0) {
>   		dev_err(dev, "failed to init streams: %d\n", ret);
> -		goto err_init_streams;
> +		goto err_ml_capabilities;
>   	}
>   
>   	ret = avs_hdac_acquire_irq(adev);
> @@ -515,7 +520,8 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>   err_acquire_irq:
>   	snd_hdac_bus_free_stream_pages(bus);
>   	snd_hdac_ext_stream_free_all(bus);
> -err_init_streams:
> +err_ml_capabilities:
> +	snd_hdac_ext_link_free_all(bus);
>   	iounmap(adev->dsp_ba);
>   err_remap_bar4:
>   	iounmap(bus->remap_addr);

Re: [PATCH v2] ASoC: intel: avs: Fix potential memory leak in avs_pci_probe()

Posted by Cezary Rojewski 2 months, 2 weeks ago

On 2025-11-13 2:11 PM, Cezary Rojewski wrote:
> On 2025-11-13 1:04 PM, Abdun Nihaal wrote:
>> The link resources allocated in snd_hdac_ext_bus_get_ml_capabilities()
>> are not freed on subsequent error paths in avs_pci_probe().

...
>> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
>> index 6e0e65584c7f..f0d77f3f3a28 100644
>> --- a/sound/soc/intel/avs/core.c
>> +++ b/sound/soc/intel/avs/core.c
>> @@ -473,8 +473,13 @@ static int avs_pci_probe(struct pci_dev *pci, 
>> const struct pci_device_id *id)
>>       }
>>       snd_hdac_bus_parse_capabilities(bus);
>> -    if (bus->mlcap)
>> -        snd_hdac_ext_bus_get_ml_capabilities(bus);
>> +    if (bus->mlcap) {
>> +        ret = snd_hdac_ext_bus_get_ml_capabilities(bus);
> 
> After giving this a second thought, I believe 
> snd_hdac_ext_bus_get_ml_capabilities() is the offender here - the 
> function should have freed whatever its already allocated before 
> returning an error, not count on the caller to free the resources 
> instead. In other words, the fix should update the callee too.
> 
> However, one may say that it's a separate issue. I'm fine with existing 
> patch landing as-is. Can prepare separate a change that covers problem 
> mentioned by me above. The cons is: additional 1-2 LOC traffic for the 
> avs-driver code.
> 
> I leave the decision to Mark, I'm OK with both approaches.

Friendly reminder. Which option do you prefer, Mark?

> 
>> +        if (ret) {
>> +            dev_err(dev, "failed to get multilink capabilities: 
>> %d\n", ret);
>> +            goto err_ml_capabilities;
>> +        }
>> +    }
>>       if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
>>           dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));