[PATCH] ALSA: ac97: Fix possible NULL dereference in snd_ac97_mixer
smatch error: sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error: we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*") Signed-off-by: Su Hui suhui@nfschina.com --- sound/pci/ac97/ac97_codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 9afc5906d662..80a65b8ad7b9 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -2069,8 +2069,8 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template, .dev_disconnect = snd_ac97_dev_disconnect, };
- if (rac97) - *rac97 = NULL; + if (!rac97) + return -EINVAL; if (snd_BUG_ON(!bus || !template)) return -EINVAL; if (snd_BUG_ON(template->num >= 4))
On Thu, 15 Jun 2023 04:17:32 +0200, Su Hui wrote:
smatch error: sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error: we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*") Signed-off-by: Su Hui suhui@nfschina.com
Thanks, applied.
Takashi
Le 15/06/2023 à 04:17, Su Hui a écrit :
smatch error: sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error: we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Hi,
why is the assigment redundant?
Should an error occur, the 'struct snd_ac97 **' parameter was garanted to be set to NULL, now it is left as-is.
I've checked all callers and apparently this is fine because the probes fail if snd_ac97_mixer() returns an error.
However, some drivers with several mixers seem to rely on the value being NULL in case of error.
See [1] as an example of such code that forces a NULL value on its own, to be sure.
So, wouldn't it be safer to leave a "*rac97 = NULL;" just after the added sanity check?
CJ
[1]: https://elixir.bootlin.com/linux/v6.5-rc7/source/sound/pci/atiixp.c#L1438
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*") Signed-off-by: Su Hui suhui@nfschina.com
sound/pci/ac97/ac97_codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 9afc5906d662..80a65b8ad7b9 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -2069,8 +2069,8 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template, .dev_disconnect = snd_ac97_dev_disconnect, };
- if (rac97)
*rac97 = NULL;
- if (!rac97)
return -EINVAL;
On Tue, 22 Aug 2023 22:07:40 +0200, Christophe JAILLET wrote:
Le 15/06/2023 à 04:17, Su Hui a écrit :
smatch error: sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error: we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Hi,
why is the assigment redundant?
It's misleading, yeah. Basically all callers are with non-NULL, hence we took rather make it mandatory. Maybe it should have been with WARN_ON() to catch the NULL argument for an out-of-tree stuff.
Should an error occur, the 'struct snd_ac97 **' parameter was garanted to be set to NULL, now it is left as-is.
I've checked all callers and apparently this is fine because the probes fail if snd_ac97_mixer() returns an error.
However, some drivers with several mixers seem to rely on the value being NULL in case of error.
See [1] as an example of such code that forces a NULL value on its own, to be sure.
So, wouldn't it be safer to leave a "*rac97 = NULL;" just after the added sanity check?
Yes, we need the NULL initialization. Care to submit an additional fix patch?
thanks,
Takashi
CJ
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*") Signed-off-by: Su Hui suhui@nfschina.com
sound/pci/ac97/ac97_codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 9afc5906d662..80a65b8ad7b9 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -2069,8 +2069,8 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template, .dev_disconnect = snd_ac97_dev_disconnect, };
- if (rac97)
*rac97 = NULL;
- if (!rac97)
return -EINVAL;
Le 23/08/2023 à 16:37, Takashi Iwai a écrit :
On Tue, 22 Aug 2023 22:07:40 +0200, Christophe JAILLET wrote:
Le 15/06/2023 à 04:17, Su Hui a écrit :
smatch error: sound/pci/ac97/ac97_codec.c:2354 snd_ac97_mixer() error: we previously assumed 'rac97' could be null (see line 2072)
remove redundant assignment, return error if rac97 is NULL.
Hi,
why is the assigment redundant?
It's misleading, yeah. Basically all callers are with non-NULL, hence we took rather make it mandatory. Maybe it should have been with WARN_ON() to catch the NULL argument for an out-of-tree stuff.
Should an error occur, the 'struct snd_ac97 **' parameter was garanted to be set to NULL, now it is left as-is.
I've checked all callers and apparently this is fine because the probes fail if snd_ac97_mixer() returns an error.
However, some drivers with several mixers seem to rely on the value being NULL in case of error.
See [1] as an example of such code that forces a NULL value on its own, to be sure.
So, wouldn't it be safer to leave a "*rac97 = NULL;" just after the added sanity check?
Yes, we need the NULL initialization. Care to submit an additional fix patch?
Hi,
Su Hui already did.
CJ
thanks,
Takashi
CJ
Fixes: da3cec35dd3c ("ALSA: Kill snd_assert() in sound/pci/*") Signed-off-by: Su Hui suhui@nfschina.com
sound/pci/ac97/ac97_codec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/ac97/ac97_codec.c b/sound/pci/ac97/ac97_codec.c index 9afc5906d662..80a65b8ad7b9 100644 --- a/sound/pci/ac97/ac97_codec.c +++ b/sound/pci/ac97/ac97_codec.c @@ -2069,8 +2069,8 @@ int snd_ac97_mixer(struct snd_ac97_bus *bus, struct snd_ac97_template *template, .dev_disconnect = snd_ac97_dev_disconnect, };
- if (rac97)
*rac97 = NULL;
- if (!rac97)
return -EINVAL;
participants (3)
-
Christophe JAILLET -
Su Hui -
Takashi Iwai