[alsa-devel] [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths
From: Colin Ian King colin.king@canonical.com
It is possible for the call to snd_hda_get_num_conns to fail and return a negative error code that gets assigned to num_conns. In that specific case, the check of very large values of val against num_conns will not fail the -EINVAL check and later on an out of bounds array read on spec->smux_paths will occur. Fix this by sanity checking for an error return from the call to snd_hda_get_num_conns.
Addresses-Coverity: ("Out-of-bounds read") Fixes: 272f3ea31776 ("ALSA: hda - Add SPDIF mux control to AD codec auto-parser") Signed-off-by: Colin Ian King colin.king@canonical.com --- sound/pci/hda/patch_analog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c index 88c46b051d14..399561369495 100644 --- a/sound/pci/hda/patch_analog.c +++ b/sound/pci/hda/patch_analog.c @@ -756,9 +756,11 @@ static int ad1988_auto_smux_enum_put(struct snd_kcontrol *kcontrol, struct ad198x_spec *spec = codec->spec; unsigned int val = ucontrol->value.enumerated.item[0]; struct nid_path *path; - int num_conns = snd_hda_get_num_conns(codec, 0x0b) + 1; + int num_conns = snd_hda_get_num_conns(codec, 0x0b);
- if (val >= num_conns) + if (num_conns < 0) + return num_conns; + if (val >= num_conns + 1) return -EINVAL; if (spec->cur_smux == val) return 0;
On Tue, 14 Jan 2020 16:44:12 +0100, Colin King wrote:
From: Colin Ian King colin.king@canonical.com
It is possible for the call to snd_hda_get_num_conns to fail and return a negative error code that gets assigned to num_conns. In that specific case, the check of very large values of val against num_conns will not fail the -EINVAL check and later on an out of bounds array read on spec->smux_paths will occur. Fix this by sanity checking for an error return from the call to snd_hda_get_num_conns.
Thanks for the patch, but this can't happen. The ad1988_auto_smux_enum_put() is used only for IEC958 Playback Source element, and it's added in ad1988_add_spdif_mux_ctl(). And there at the beginning, there is already a check of the value:
num_conns = snd_hda_get_num_conns(codec, 0x0b) + 1; if (num_conns != 3 && num_conns != 4) return 0; And the snd_hda_get_num_conns() function returns the cached value, hence it's always same at the second and later calls, so it can't be a negative error.
That said, I don't think we need to apply the change as is. But if we were to improve something, we can rather record this number more explicitly e.g. introduce a new field spec->num_spdif_mux_conns and keep there instead of calling snd_hda_get_num_conns() at each place.
thanks,
Takashi
Addresses-Coverity: ("Out-of-bounds read") Fixes: 272f3ea31776 ("ALSA: hda - Add SPDIF mux control to AD codec auto-parser") Signed-off-by: Colin Ian King colin.king@canonical.com
sound/pci/hda/patch_analog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c index 88c46b051d14..399561369495 100644 --- a/sound/pci/hda/patch_analog.c +++ b/sound/pci/hda/patch_analog.c @@ -756,9 +756,11 @@ static int ad1988_auto_smux_enum_put(struct snd_kcontrol *kcontrol, struct ad198x_spec *spec = codec->spec; unsigned int val = ucontrol->value.enumerated.item[0]; struct nid_path *path;
- int num_conns = snd_hda_get_num_conns(codec, 0x0b) + 1;
- int num_conns = snd_hda_get_num_conns(codec, 0x0b);
- if (val >= num_conns)
- if (num_conns < 0)
return num_conns;- if (val >= num_conns + 1) return -EINVAL; if (spec->cur_smux == val) return 0;
-- 2.24.0
On 14/01/2020 20:01, Takashi Iwai wrote:
On Tue, 14 Jan 2020 16:44:12 +0100, Colin King wrote:
From: Colin Ian King colin.king@canonical.com
It is possible for the call to snd_hda_get_num_conns to fail and return a negative error code that gets assigned to num_conns. In that specific case, the check of very large values of val against num_conns will not fail the -EINVAL check and later on an out of bounds array read on spec->smux_paths will occur. Fix this by sanity checking for an error return from the call to snd_hda_get_num_conns.
Thanks for the patch, but this can't happen. The ad1988_auto_smux_enum_put() is used only for IEC958 Playback Source element, and it's added in ad1988_add_spdif_mux_ctl(). And there at the beginning, there is already a check of the value:
num_conns = snd_hda_get_num_conns(codec, 0x0b) + 1; if (num_conns != 3 && num_conns != 4) return 0; And the snd_hda_get_num_conns() function returns the cached value, hence it's always same at the second and later calls, so it can't be a negative error.
Ah, OK, sorry about the noise.
That said, I don't think we need to apply the change as is. But if we were to improve something, we can rather record this number more explicitly e.g. introduce a new field spec->num_spdif_mux_conns and keep there instead of calling snd_hda_get_num_conns() at each place.
That would seem more optimal for sure.
Colin
thanks,
Takashi
Addresses-Coverity: ("Out-of-bounds read") Fixes: 272f3ea31776 ("ALSA: hda - Add SPDIF mux control to AD codec auto-parser") Signed-off-by: Colin Ian King colin.king@canonical.com
sound/pci/hda/patch_analog.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c index 88c46b051d14..399561369495 100644 --- a/sound/pci/hda/patch_analog.c +++ b/sound/pci/hda/patch_analog.c @@ -756,9 +756,11 @@ static int ad1988_auto_smux_enum_put(struct snd_kcontrol *kcontrol, struct ad198x_spec *spec = codec->spec; unsigned int val = ucontrol->value.enumerated.item[0]; struct nid_path *path;
- int num_conns = snd_hda_get_num_conns(codec, 0x0b) + 1;
- int num_conns = snd_hda_get_num_conns(codec, 0x0b);
- if (val >= num_conns)
- if (num_conns < 0)
return num_conns;- if (val >= num_conns + 1) return -EINVAL; if (spec->cur_smux == val) return 0;
-- 2.24.0
participants (3)
-
Colin Ian King -
Colin King
-
Takashi Iwai