* [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths
@ 2020-01-14 15:44 Colin King
2020-01-14 20:01 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Colin King @ 2020-01-14 15:44 UTC (permalink / raw)
To: Jaroslav Kysela, Takashi Iwai, alsa-devel; +Cc: kernel-janitors, linux-kernel
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;
--
2.24.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths 2020-01-14 15:44 [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths Colin King @ 2020-01-14 20:01 ` Takashi Iwai 2020-01-14 20:05 ` Colin Ian King 0 siblings, 1 reply; 3+ messages in thread From: Takashi Iwai @ 2020-01-14 20:01 UTC (permalink / raw) To: Colin King; +Cc: Jaroslav Kysela, alsa-devel, kernel-janitors, linux-kernel 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths 2020-01-14 20:01 ` Takashi Iwai @ 2020-01-14 20:05 ` Colin Ian King 0 siblings, 0 replies; 3+ messages in thread From: Colin Ian King @ 2020-01-14 20:05 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jaroslav Kysela, alsa-devel, kernel-janitors, linux-kernel 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 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-14 20:05 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-14 15:44 [PATCH] ALSA: hda - fix out of bounds read on spec->smux_paths Colin King 2020-01-14 20:01 ` Takashi Iwai 2020-01-14 20:05 ` Colin Ian King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).