[PATCH] ASoc: PCM6240: Fix a null pointer dereference in pcmdevice_i2c_probe

[PATCH] ASoc: PCM6240: Fix a null pointer dereference in pcmdevice_i2c_probe

[Hao Ge]

From: Hao Ge <gehao@kylinos.cn>

When devm_kzalloc return NULL,we return -ENOMEM directly to
avoid a null pointer that call pcmdevice_remove which will
perform some operations on the members of the pcm_dev;

Fixes: 1324eafd37aa ("ASoc: PCM6240: Create PCM6240 Family driver code")
Signed-off-by: Hao Ge <gehao@kylinos.cn>
---
 sound/soc/codecs/pcm6240.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm6240.c b/sound/soc/codecs/pcm6240.c
index 86e126783a1d..d3adcea457a2 100644
--- a/sound/soc/codecs/pcm6240.c
+++ b/sound/soc/codecs/pcm6240.c
@@ -2088,8 +2088,7 @@ static int pcmdevice_i2c_probe(struct i2c_client *i2c)
 
 	pcm_dev = devm_kzalloc(&i2c->dev, sizeof(*pcm_dev), GFP_KERNEL);
 	if (!pcm_dev) {
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	pcm_dev->chip_id = (id != NULL) ? id->driver_data : 0;
-- 
2.25.1

Re: [PATCH] ASoc: PCM6240: Fix a null pointer dereference in pcmdevice_i2c_probe

[Markus Elfring]

> When devm_kzalloc return NULL,we return -ENOMEM directly to

This information fits to a coding style advice.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.10-rc3#n532


> avoid a null pointer that call pcmdevice_remove which will
> perform some operations on the members of the pcm_dev;

You pointed in an improvable way out that the label “out” was questionable
after the assignment of a well-known error code at the beginning.
https://elixir.bootlin.com/linux/v6.10-rc3/source/sound/soc/codecs/pcm6240.c#L2077


Can a wording approach (like the following) be a better change description?

   The value “-ENOMEM” was assigned to the local variable “ret”
   in one if branch after a devm_kzalloc() call failed at the beginning.
   This error code will trigger then a pcmdevice_remove() call
   with a passed null pointer so that an undesirable dereference
   will be performed.
   Thus return the appropriate error code directly.


Can a summary phrase like “Return directly after a failed devm_kzalloc()
in pcmdevice_i2c_probe()” be also helpful?

Regards,
Markus

Re: [PATCH] ASoc: PCM6240: Fix a null pointer dereference in pcmdevice_i2c_probe

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Fri, Jun 14, 2024 at 10:20:38PM +0200, Markus Elfring wrote:
> > When devm_kzalloc return NULL,we return -ENOMEM directly to
> 
> This information fits to a coding style advice.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.10-rc3#n532

Feel free to ignore Markus, he has a long history of sending
unhelpful review comments and continues to ignore repeated requests
to stop.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoc: PCM6240: Fix a null pointer dereference in pcmdevice_i2c_probe

[Markus Elfring]

…
> +++ b/sound/soc/codecs/pcm6240.c
> @@ -2088,8 +2088,7 @@ static int pcmdevice_i2c_probe(struct i2c_client *i2c)
>
>  	pcm_dev = devm_kzalloc(&i2c->dev, sizeof(*pcm_dev), GFP_KERNEL);
>  	if (!pcm_dev) {
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
…

Would you like to omit curly brackets from a single if branch?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.10-rc3#n197

Regards,
Markus

[PATCH v2] ASoc: PCM6240: Return directly after a failed devm_kzalloc() in pcmdevice_i2c_probe()

[Hao Ge]

From: Hao Ge <gehao@kylinos.cn>

The value “-ENOMEM” was assigned to the local variable “ret”
in one if branch after a devm_kzalloc() call failed at the beginning.
This error code will trigger then a pcmdevice_remove() call with a passed
null pointer so that an undesirable dereference will be performed.
Thus return the appropriate error code directly.

Fixes: 1324eafd37aa ("ASoc: PCM6240: Create PCM6240 Family driver code")
Signed-off-by: Hao Ge <gehao@kylinos.cn>

---
v2:
- adjust title and commit message
- omit curly brackets
---
 sound/soc/codecs/pcm6240.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/pcm6240.c b/sound/soc/codecs/pcm6240.c
index 86e126783a1d..8f7057e689fb 100644
--- a/sound/soc/codecs/pcm6240.c
+++ b/sound/soc/codecs/pcm6240.c
@@ -2087,10 +2087,8 @@ static int pcmdevice_i2c_probe(struct i2c_client *i2c)
 #endif
 
 	pcm_dev = devm_kzalloc(&i2c->dev, sizeof(*pcm_dev), GFP_KERNEL);
-	if (!pcm_dev) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!pcm_dev)
+		return -ENOMEM;
 
 	pcm_dev->chip_id = (id != NULL) ? id->driver_data : 0;
 
-- 
2.25.1

Re: [PATCH v2] ASoc: PCM6240: Return directly after a failed devm_kzalloc() in pcmdevice_i2c_probe()

[Mark Brown]

On Mon, 17 Jun 2024 10:09:54 +0800, Hao Ge wrote:
> The value “-ENOMEM” was assigned to the local variable “ret”
> in one if branch after a devm_kzalloc() call failed at the beginning.
> This error code will trigger then a pcmdevice_remove() call with a passed
> null pointer so that an undesirable dereference will be performed.
> Thus return the appropriate error code directly.
> 
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoc: PCM6240: Return directly after a failed devm_kzalloc() in pcmdevice_i2c_probe()
      commit: 3722873d49a1788d5420894d4f6f63e35f5c1f13

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark