* [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
@ 2019-08-17 4:32 Hui Peng
2019-08-17 6:55 ` Takashi Iwai
0 siblings, 1 reply; 10+ messages in thread
From: Hui Peng @ 2019-08-17 4:32 UTC (permalink / raw)
To: security
Cc: Hui Peng, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
Wenwen Wang, Thomas Gleixner, YueHaibing, alsa-devel,
linux-kernel
`uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
to get pointer to bmControls field. The current implementation of
`uac_mixer_unit_get_channels` does properly check the size of
uac_mixer_unit_descriptor descriptor and may allow OOB access
in `uac_mixer_unit_bmControls`.
Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
Signed-off-by: Hui Peng <benquike@gmail.com>
---
sound/usb/mixer.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b5927c3d5bc0..00e6274a63c3 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct mixer_build *state, unsigned int clust
static int uac_mixer_unit_get_channels(struct mixer_build *state,
struct uac_mixer_unit_descriptor *desc)
{
- int mu_channels;
+ int mu_channels = 0;
void *c;
- if (desc->bLength < sizeof(*desc))
- return -EINVAL;
if (!desc->bNrInPins)
return -EINVAL;
- if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
- return -EINVAL;
switch (state->mixer->protocol) {
case UAC_VERSION_1:
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
+ return 0;
+
+ mu_channels = uac_mixer_unit_bNrChannels(desc);
+ break;
+
case UAC_VERSION_2:
- default:
- if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
return 0; /* no bmControls -> skip */
+
mu_channels = uac_mixer_unit_bNrChannels(desc);
break;
case UAC_VERSION_3:
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
+ return 0; /* no bmControls -> skip */
+
mu_channels = get_cluster_channels_v3(state,
uac3_mixer_unit_wClusterDescrID(desc));
break;
+
+ default:
+ break;
}
if (!mu_channels)
--
2.22.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-17 4:32 [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls Hui Peng
@ 2019-08-17 6:55 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-08-17 6:55 UTC (permalink / raw)
To: Hui Peng
Cc: security, alsa-devel, YueHaibing, Thomas Gleixner, Mathias Payer,
Jaroslav Kysela, Takashi Iwai, Wenwen Wang, linux-kernel
On Sat, 17 Aug 2019 06:32:07 +0200,
Hui Peng wrote:
>
> `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> to get pointer to bmControls field. The current implementation of
> `uac_mixer_unit_get_channels` does properly check the size of
> uac_mixer_unit_descriptor descriptor and may allow OOB access
> in `uac_mixer_unit_bmControls`.
>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Hui Peng <benquike@gmail.com>
Ah a good catch.
One easier fix in this case would be to get the offset from
uac_mixer_unit_bmControls(), e.g.
/* calculate the offset of bmControls field */
size_t bmc_offset = uac_mixer_unit_bmControls(NULL, protocol) - NULL;
....
if (desc->bLength < bmc_offset)
return 0;
thanks,
Takashi
> ---
> sound/usb/mixer.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index b5927c3d5bc0..00e6274a63c3 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct mixer_build *state, unsigned int clust
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor *desc)
> {
> - int mu_channels;
> + int mu_channels = 0;
> void *c;
>
> - if (desc->bLength < sizeof(*desc))
> - return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> - return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> + return 0;
> +
> + mu_channels = uac_mixer_unit_bNrChannels(desc);
> + break;
> +
> case UAC_VERSION_2:
> - default:
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> return 0; /* no bmControls -> skip */
> +
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> + return 0; /* no bmControls -> skip */
> +
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> +
> + default:
> + break;
> }
>
> if (!mu_channels)
> --
> 2.22.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
@ 2019-08-17 6:55 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-08-17 6:55 UTC (permalink / raw)
To: Hui Peng
Cc: security, alsa-devel, YueHaibing, Thomas Gleixner, Mathias Payer,
Jaroslav Kysela, Takashi Iwai, Wenwen Wang, linux-kernel
On Sat, 17 Aug 2019 06:32:07 +0200,
Hui Peng wrote:
>
> `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> to get pointer to bmControls field. The current implementation of
> `uac_mixer_unit_get_channels` does properly check the size of
> uac_mixer_unit_descriptor descriptor and may allow OOB access
> in `uac_mixer_unit_bmControls`.
>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Hui Peng <benquike@gmail.com>
Ah a good catch.
One easier fix in this case would be to get the offset from
uac_mixer_unit_bmControls(), e.g.
/* calculate the offset of bmControls field */
size_t bmc_offset = uac_mixer_unit_bmControls(NULL, protocol) - NULL;
....
if (desc->bLength < bmc_offset)
return 0;
thanks,
Takashi
> ---
> sound/usb/mixer.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index b5927c3d5bc0..00e6274a63c3 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct mixer_build *state, unsigned int clust
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor *desc)
> {
> - int mu_channels;
> + int mu_channels = 0;
> void *c;
>
> - if (desc->bLength < sizeof(*desc))
> - return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> - return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> + return 0;
> +
> + mu_channels = uac_mixer_unit_bNrChannels(desc);
> + break;
> +
> case UAC_VERSION_2:
> - default:
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> return 0; /* no bmControls -> skip */
> +
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> + return 0; /* no bmControls -> skip */
> +
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> +
> + default:
> + break;
> }
>
> if (!mu_channels)
> --
> 2.22.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-17 6:55 ` Takashi Iwai
(?)
@ 2019-08-17 15:57 ` Hui Peng
2019-08-17 16:18 ` Takashi Iwai
-1 siblings, 1 reply; 10+ messages in thread
From: Hui Peng @ 2019-08-17 15:57 UTC (permalink / raw)
To: Takashi Iwai
Cc: Mathias Payer, Wenwen Wang, alsa-devel, linux-kernel,
Takashi Iwai, YueHaibing, security, Thomas Gleixner
Looking around, there are other suspicious codes. E.g., in the following
function, it seems to be the same as `uac_mixer_unit_bmControls`, but it is
accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
Is this intended?
static inline __u8
<https://elixir.bootlin.com/linux/latest/ident/__u8>
*uac_processing_unit_bmControls
<https://elixir.bootlin.com/linux/latest/ident/uac_processing_unit_bmControls>(struct
uac_processing_unit_descriptor
<https://elixir.bootlin.com/linux/latest/ident/uac_processing_unit_descriptor>
*desc,
int protocol <https://elixir.bootlin.com/linux/latest/ident/protocol>){
switch (protocol <https://elixir.bootlin.com/linux/latest/ident/protocol>) {
case UAC_VERSION_1
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_1>:
return &desc->baSourceID[desc->bNrInPins + 5];
case UAC_VERSION_2
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_2>:
return &desc->baSourceID[desc->bNrInPins + 6];
case UAC_VERSION_3
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_3>:
return &desc->baSourceID[desc->bNrInPins + 2];
default:
return NULL;
}}
On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Sat, 17 Aug 2019 06:32:07 +0200,
> Hui Peng wrote:
> >
> > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > to get pointer to bmControls field. The current implementation of
> > `uac_mixer_unit_get_channels` does properly check the size of
> > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > in `uac_mixer_unit_bmControls`.
> >
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > Signed-off-by: Hui Peng <benquike@gmail.com>
>
> Ah a good catch.
>
> One easier fix in this case would be to get the offset from
> uac_mixer_unit_bmControls(), e.g.
>
> /* calculate the offset of bmControls field */
> size_t bmc_offset = uac_mixer_unit_bmControls(NULL, protocol) -
> NULL;
> ....
> if (desc->bLength < bmc_offset)
> return 0;
>
> thanks,
>
> Takashi
>
>
> > ---
> > sound/usb/mixer.c | 25 ++++++++++++++++++-------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index b5927c3d5bc0..00e6274a63c3 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> mixer_build *state, unsigned int clust
> > static int uac_mixer_unit_get_channels(struct mixer_build *state,
> > struct uac_mixer_unit_descriptor
> *desc)
> > {
> > - int mu_channels;
> > + int mu_channels = 0;
> > void *c;
> >
> > - if (desc->bLength < sizeof(*desc))
> > - return -EINVAL;
> > if (!desc->bNrInPins)
> > return -EINVAL;
> > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > - return -EINVAL;
> >
> > switch (state->mixer->protocol) {
> > case UAC_VERSION_1:
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> > + return 0;
> > +
> > + mu_channels = uac_mixer_unit_bNrChannels(desc);
> > + break;
> > +
> > case UAC_VERSION_2:
> > - default:
> > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> > return 0; /* no bmControls -> skip */
> > +
> > mu_channels = uac_mixer_unit_bNrChannels(desc);
> > break;
> > case UAC_VERSION_3:
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> > + return 0; /* no bmControls -> skip */
> > +
> > mu_channels = get_cluster_channels_v3(state,
> > uac3_mixer_unit_wClusterDescrID(desc));
> > break;
> > +
> > + default:
> > + break;
> > }
> >
> > if (!mu_channels)
> > --
> > 2.22.1
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-17 15:57 ` Hui Peng
@ 2019-08-17 16:18 ` Takashi Iwai
2019-08-17 16:47 ` Hui Peng
0 siblings, 1 reply; 10+ messages in thread
From: Takashi Iwai @ 2019-08-17 16:18 UTC (permalink / raw)
To: Hui Peng
Cc: security, alsa-devel, YueHaibing, Thomas Gleixner, Mathias Payer,
Jaroslav Kysela, Takashi Iwai, Wenwen Wang, linux-kernel
On Sat, 17 Aug 2019 17:57:38 +0200,
Hui Peng wrote:
>
> Looking around, there are other suspicious codes. E.g., in the following
> function, it seems to be the same as `uac_mixer_unit_bmControls`, but it is
> accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
> Is this intended?
Yes, this isn't for the mixer unit but for the processing unit.
They have different definitions.
Now back to the original report: I read the code again but fail to see
where is OOB access. Let's see the function:
static int uac_mixer_unit_get_channels(struct mixer_build *state,
struct uac_mixer_unit_descriptor *desc)
{
int mu_channels;
void *c;
if (desc->bLength < sizeof(*desc))
return -EINVAL;
if (!desc->bNrInPins)
return -EINVAL;
if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
return -EINVAL;
switch (state->mixer->protocol) {
case UAC_VERSION_1:
case UAC_VERSION_2:
default:
if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
return 0; /* no bmControls -> skip */
mu_channels = uac_mixer_unit_bNrChannels(desc);
break;
case UAC_VERSION_3:
mu_channels = get_cluster_channels_v3(state,
uac3_mixer_unit_wClusterDescrID(desc));
break;
}
if (!mu_channels)
return 0;
... until this point, mu_channels is calculated but no actual access
happens. Then:
c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
... this returns the *address* of the bmControls bitmap. At this
point, it's not accessed yet. Now:
if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
return 0; /* no bmControls -> skip */
... here we check whether the actual bitmap address plus the max
bitmap size overflows bLength. And if it overflows, returns 0,
indicating no bitmap available.
So the code doesn't access but checks properly beforehand as far as I
understand. Is the actual OOB access triggered by some program?
thanks,
Takashi
>
> static inline __u8 *uac_processing_unit_bmControls(struct uac_processing_unit_descriptor *desc,
> int protocol)
> {
> switch (protocol) {
> case UAC_VERSION_1:
> return &desc->baSourceID[desc->bNrInPins + 5];
> case UAC_VERSION_2:
> return &desc->baSourceID[desc->bNrInPins + 6];
> case UAC_VERSION_3:
> return &desc->baSourceID[desc->bNrInPins + 2];
> default:
> return NULL;
> }
> }
>
> On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sat, 17 Aug 2019 06:32:07 +0200,
> Hui Peng wrote:
> >
> > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > to get pointer to bmControls field. The current implementation of
> > `uac_mixer_unit_get_channels` does properly check the size of
> > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > in `uac_mixer_unit_bmControls`.
> >
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > Signed-off-by: Hui Peng <benquike@gmail.com>
>
> Ah a good catch.
>
> One easier fix in this case would be to get the offset from
> uac_mixer_unit_bmControls(), e.g.
>
> /* calculate the offset of bmControls field */
> size_t bmc_offset = uac_mixer_unit_bmControls(NULL, protocol) -
> NULL;
> ....
> if (desc->bLength < bmc_offset)
> return 0;
>
> thanks,
>
> Takashi
>
> > ---
> > sound/usb/mixer.c | 25 ++++++++++++++++++-------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index b5927c3d5bc0..00e6274a63c3 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> mixer_build *state, unsigned int clust
> > static int uac_mixer_unit_get_channels(struct mixer_build *state,
> > struct uac_mixer_unit_descriptor
> *desc)
> > {
> > - int mu_channels;
> > + int mu_channels = 0;
> > void *c;
> >
> > - if (desc->bLength < sizeof(*desc))
> > - return -EINVAL;
> > if (!desc->bNrInPins)
> > return -EINVAL;
> > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > - return -EINVAL;
> >
> > switch (state->mixer->protocol) {
> > case UAC_VERSION_1:
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> > + return 0;
> > +
> > + mu_channels = uac_mixer_unit_bNrChannels(desc);
> > + break;
> > +
> > case UAC_VERSION_2:
> > - default:
> > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> > return 0; /* no bmControls -> skip */
> > +
> > mu_channels = uac_mixer_unit_bNrChannels(desc);
> > break;
> > case UAC_VERSION_3:
> > + // limit derived from uac_mixer_unit_bmControls
> > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> > + return 0; /* no bmControls -> skip */
> > +
> > mu_channels = get_cluster_channels_v3(state,
> > uac3_mixer_unit_wClusterDescrID(desc));
> > break;
> > +
> > + default:
> > + break;
> > }
> >
> > if (!mu_channels)
> > --
> > 2.22.1
> >
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-17 16:18 ` Takashi Iwai
@ 2019-08-17 16:47 ` Hui Peng
2019-08-17 17:59 ` Takashi Iwai
0 siblings, 1 reply; 10+ messages in thread
From: Hui Peng @ 2019-08-17 16:47 UTC (permalink / raw)
To: Takashi Iwai
Cc: Mathias Payer, Wenwen Wang, alsa-devel, linux-kernel,
Takashi Iwai, YueHaibing, security, Thomas Gleixner
No, there was not triggering. I found it accidentally when I was going
through the code.
Yeah, you are right. it is handled in the last check. Is it defined in the
spec that the descriptor needs to have 4/6/2 additional bytes for the
bmControl field?, if so, it is easier to understand the using the code in
the way in my first patch.
If you think this is unnecessary, we can skip this patch.
On Sat, Aug 17, 2019 at 12:18 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Sat, 17 Aug 2019 17:57:38 +0200,
> Hui Peng wrote:
> >
> > Looking around, there are other suspicious codes. E.g., in the following
> > function, it seems to be the same as `uac_mixer_unit_bmControls`, but it
> is
> > accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
> > Is this intended?
>
> Yes, this isn't for the mixer unit but for the processing unit.
> They have different definitions.
>
> Now back to the original report: I read the code again but fail to see
> where is OOB access. Let's see the function:
>
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor
> *desc)
> {
> int mu_channels;
> void *c;
>
> if (desc->bLength < sizeof(*desc))
> return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> case UAC_VERSION_2:
> default:
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> return 0; /* no bmControls -> skip */
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> }
>
> if (!mu_channels)
> return 0;
>
> ... until this point, mu_channels is calculated but no actual access
> happens. Then:
>
> c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
>
> ... this returns the *address* of the bmControls bitmap. At this
> point, it's not accessed yet. Now:
>
> if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
> return 0; /* no bmControls -> skip */
>
> ... here we check whether the actual bitmap address plus the max
> bitmap size overflows bLength. And if it overflows, returns 0,
> indicating no bitmap available.
>
> So the code doesn't access but checks properly beforehand as far as I
> understand. Is the actual OOB access triggered by some program?
>
>
> thanks,
>
> Takashi
>
> >
> > static inline __u8 *uac_processing_unit_bmControls(struct
> uac_processing_unit_descriptor *desc,
> > int protocol)
> > {
> > switch (protocol) {
> > case UAC_VERSION_1:
> > return &desc->baSourceID[desc->bNrInPins + 5];
> > case UAC_VERSION_2:
> > return &desc->baSourceID[desc->bNrInPins + 6];
> > case UAC_VERSION_3:
> > return &desc->baSourceID[desc->bNrInPins + 2];
> > default:
> > return NULL;
> > }
> > }
> >
> > On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sat, 17 Aug 2019 06:32:07 +0200,
> > Hui Peng wrote:
> > >
> > > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > > to get pointer to bmControls field. The current implementation of
> > > `uac_mixer_unit_get_channels` does properly check the size of
> > > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > > in `uac_mixer_unit_bmControls`.
> > >
> > > Reported-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > Signed-off-by: Hui Peng <benquike@gmail.com>
> >
> > Ah a good catch.
> >
> > One easier fix in this case would be to get the offset from
> > uac_mixer_unit_bmControls(), e.g.
> >
> > /* calculate the offset of bmControls field */
> > size_t bmc_offset = uac_mixer_unit_bmControls(NULL,
> protocol) -
> > NULL;
> > ....
> > if (desc->bLength < bmc_offset)
> > return 0;
> >
> > thanks,
> >
> > Takashi
> >
> > > ---
> > > sound/usb/mixer.c | 25 ++++++++++++++++++-------
> > > 1 file changed, 18 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index b5927c3d5bc0..00e6274a63c3 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> > mixer_build *state, unsigned int clust
> > > static int uac_mixer_unit_get_channels(struct mixer_build *state,
> > > struct
> uac_mixer_unit_descriptor
> > *desc)
> > > {
> > > - int mu_channels;
> > > + int mu_channels = 0;
> > > void *c;
> > >
> > > - if (desc->bLength < sizeof(*desc))
> > > - return -EINVAL;
> > > if (!desc->bNrInPins)
> > > return -EINVAL;
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > > - return -EINVAL;
> > >
> > > switch (state->mixer->protocol) {
> > > case UAC_VERSION_1:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 4)
> > > + return 0;
> > > +
> > > + mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > + break;
> > > +
> > > case UAC_VERSION_2:
> > > - default:
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 1)
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 6)
> > > return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > break;
> > > case UAC_VERSION_3:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 2)
> > > + return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = get_cluster_channels_v3(state,
> > >
> uac3_mixer_unit_wClusterDescrID(desc));
> > > break;
> > > +
> > > + default:
> > > + break;
> > > }
> > >
> > > if (!mu_channels)
> > > --
> > > 2.22.1
> > >
> > >
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-17 16:47 ` Hui Peng
@ 2019-08-17 17:59 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-08-17 17:59 UTC (permalink / raw)
To: Hui Peng
Cc: Mathias Payer, Wenwen Wang, alsa-devel, linux-kernel,
Takashi Iwai, YueHaibing, security, Thomas Gleixner
On Sat, 17 Aug 2019 18:47:05 +0200,
Hui Peng wrote:
>
> No, there was not triggering. I found it accidentally when I was going through
> the code.
>
> Yeah, you are right. it is handled in the last check. Is it defined in the
> spec that the descriptor needs to have 4/6/2 additional bytes for the
> bmControl field?, if so, it is easier to understand the using the code in the
> way in my first patch.
>
> If you think this is unnecessary, we can skip this patch.
Well, the patch essentially open-codes what the helper function does
adds one more check unnecessarily, so I don't think it worth.
Let's skip it.
thanks,
Takashi
> On Sat, Aug 17, 2019 at 12:18 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sat, 17 Aug 2019 17:57:38 +0200,
> Hui Peng wrote:
> >
> > Looking around, there are other suspicious codes. E.g., in the following
> > function, it seems to be the same as `uac_mixer_unit_bmControls`, but it
> is
> > accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
> > Is this intended?
>
> Yes, this isn't for the mixer unit but for the processing unit.
> They have different definitions.
>
> Now back to the original report: I read the code again but fail to see
> where is OOB access. Let's see the function:
>
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor
> *desc)
> {
> int mu_channels;
> void *c;
>
> if (desc->bLength < sizeof(*desc))
> return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> case UAC_VERSION_2:
> default:
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> return 0; /* no bmControls -> skip */
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> }
>
> if (!mu_channels)
> return 0;
>
> ... until this point, mu_channels is calculated but no actual access
> happens. Then:
>
> c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
>
> ... this returns the *address* of the bmControls bitmap. At this
> point, it's not accessed yet. Now:
>
> if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
> return 0; /* no bmControls -> skip */
>
> ... here we check whether the actual bitmap address plus the max
> bitmap size overflows bLength. And if it overflows, returns 0,
> indicating no bitmap available.
>
> So the code doesn't access but checks properly beforehand as far as I
> understand. Is the actual OOB access triggered by some program?
>
> thanks,
>
> Takashi
>
> >
> > static inline __u8 *uac_processing_unit_bmControls(struct
> uac_processing_unit_descriptor *desc,
> > int protocol)
> > {
> > switch (protocol) {
> > case UAC_VERSION_1:
> > return &desc->baSourceID[desc->bNrInPins + 5];
> > case UAC_VERSION_2:
> > return &desc->baSourceID[desc->bNrInPins + 6];
> > case UAC_VERSION_3:
> > return &desc->baSourceID[desc->bNrInPins + 2];
> > default:
> > return NULL;
> > }
> > }
> >
> > On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sat, 17 Aug 2019 06:32:07 +0200,
> > Hui Peng wrote:
> > >
> > > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > > to get pointer to bmControls field. The current implementation of
> > > `uac_mixer_unit_get_channels` does properly check the size of
> > > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > > in `uac_mixer_unit_bmControls`.
> > >
> > > Reported-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > Signed-off-by: Hui Peng <benquike@gmail.com>
> >
> > Ah a good catch.
> >
> > One easier fix in this case would be to get the offset from
> > uac_mixer_unit_bmControls(), e.g.
> >
> > /* calculate the offset of bmControls field */
> > size_t bmc_offset = uac_mixer_unit_bmControls(NULL,
> protocol) -
> > NULL;
> > ....
> > if (desc->bLength < bmc_offset)
> > return 0;
> >
> > thanks,
> >
> > Takashi
> >
> > > ---
> > > sound/usb/mixer.c | 25 ++++++++++++++++++-------
> > > 1 file changed, 18 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index b5927c3d5bc0..00e6274a63c3 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> > mixer_build *state, unsigned int clust
> > > static int uac_mixer_unit_get_channels(struct mixer_build *state,
> > > struct
> uac_mixer_unit_descriptor
> > *desc)
> > > {
> > > - int mu_channels;
> > > + int mu_channels = 0;
> > > void *c;
> > >
> > > - if (desc->bLength < sizeof(*desc))
> > > - return -EINVAL;
> > > if (!desc->bNrInPins)
> > > return -EINVAL;
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > > - return -EINVAL;
> > >
> > > switch (state->mixer->protocol) {
> > > case UAC_VERSION_1:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 4)
> > > + return 0;
> > > +
> > > + mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > + break;
> > > +
> > > case UAC_VERSION_2:
> > > - default:
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 1)
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 6)
> > > return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > break;
> > > case UAC_VERSION_3:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 2)
> > > + return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = get_cluster_channels_v3(state,
> > > uac3_mixer_unit_wClusterDescrID
> (desc));
> > > break;
> > > +
> > > + default:
> > > + break;
> > > }
> > >
> > > if (!mu_channels)
> > > --
> > > 2.22.1
> > >
> > >
> >
> >
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
@ 2019-08-17 17:59 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2019-08-17 17:59 UTC (permalink / raw)
To: Hui Peng
Cc: security, alsa-devel, YueHaibing, Thomas Gleixner, Mathias Payer,
Jaroslav Kysela, Takashi Iwai, Wenwen Wang, linux-kernel
On Sat, 17 Aug 2019 18:47:05 +0200,
Hui Peng wrote:
>
> No, there was not triggering. I found it accidentally when I was going through
> the code.
>
> Yeah, you are right. it is handled in the last check. Is it defined in the
> spec that the descriptor needs to have 4/6/2 additional bytes for the
> bmControl field?, if so, it is easier to understand the using the code in the
> way in my first patch.
>
> If you think this is unnecessary, we can skip this patch.
Well, the patch essentially open-codes what the helper function does
adds one more check unnecessarily, so I don't think it worth.
Let's skip it.
thanks,
Takashi
> On Sat, Aug 17, 2019 at 12:18 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sat, 17 Aug 2019 17:57:38 +0200,
> Hui Peng wrote:
> >
> > Looking around, there are other suspicious codes. E.g., in the following
> > function, it seems to be the same as `uac_mixer_unit_bmControls`, but it
> is
> > accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
> > Is this intended?
>
> Yes, this isn't for the mixer unit but for the processing unit.
> They have different definitions.
>
> Now back to the original report: I read the code again but fail to see
> where is OOB access. Let's see the function:
>
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor
> *desc)
> {
> int mu_channels;
> void *c;
>
> if (desc->bLength < sizeof(*desc))
> return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> case UAC_VERSION_2:
> default:
> if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> return 0; /* no bmControls -> skip */
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> }
>
> if (!mu_channels)
> return 0;
>
> ... until this point, mu_channels is calculated but no actual access
> happens. Then:
>
> c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
>
> ... this returns the *address* of the bmControls bitmap. At this
> point, it's not accessed yet. Now:
>
> if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
> return 0; /* no bmControls -> skip */
>
> ... here we check whether the actual bitmap address plus the max
> bitmap size overflows bLength. And if it overflows, returns 0,
> indicating no bitmap available.
>
> So the code doesn't access but checks properly beforehand as far as I
> understand. Is the actual OOB access triggered by some program?
>
> thanks,
>
> Takashi
>
> >
> > static inline __u8 *uac_processing_unit_bmControls(struct
> uac_processing_unit_descriptor *desc,
> > int protocol)
> > {
> > switch (protocol) {
> > case UAC_VERSION_1:
> > return &desc->baSourceID[desc->bNrInPins + 5];
> > case UAC_VERSION_2:
> > return &desc->baSourceID[desc->bNrInPins + 6];
> > case UAC_VERSION_3:
> > return &desc->baSourceID[desc->bNrInPins + 2];
> > default:
> > return NULL;
> > }
> > }
> >
> > On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sat, 17 Aug 2019 06:32:07 +0200,
> > Hui Peng wrote:
> > >
> > > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > > to get pointer to bmControls field. The current implementation of
> > > `uac_mixer_unit_get_channels` does properly check the size of
> > > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > > in `uac_mixer_unit_bmControls`.
> > >
> > > Reported-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > Signed-off-by: Hui Peng <benquike@gmail.com>
> >
> > Ah a good catch.
> >
> > One easier fix in this case would be to get the offset from
> > uac_mixer_unit_bmControls(), e.g.
> >
> > /* calculate the offset of bmControls field */
> > size_t bmc_offset = uac_mixer_unit_bmControls(NULL,
> protocol) -
> > NULL;
> > ....
> > if (desc->bLength < bmc_offset)
> > return 0;
> >
> > thanks,
> >
> > Takashi
> >
> > > ---
> > > sound/usb/mixer.c | 25 ++++++++++++++++++-------
> > > 1 file changed, 18 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index b5927c3d5bc0..00e6274a63c3 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> > mixer_build *state, unsigned int clust
> > > static int uac_mixer_unit_get_channels(struct mixer_build *state,
> > > struct
> uac_mixer_unit_descriptor
> > *desc)
> > > {
> > > - int mu_channels;
> > > + int mu_channels = 0;
> > > void *c;
> > >
> > > - if (desc->bLength < sizeof(*desc))
> > > - return -EINVAL;
> > > if (!desc->bNrInPins)
> > > return -EINVAL;
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > > - return -EINVAL;
> > >
> > > switch (state->mixer->protocol) {
> > > case UAC_VERSION_1:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 4)
> > > + return 0;
> > > +
> > > + mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > + break;
> > > +
> > > case UAC_VERSION_2:
> > > - default:
> > > - if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 1)
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 6)
> > > return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = uac_mixer_unit_bNrChannels(desc);
> > > break;
> > > case UAC_VERSION_3:
> > > + // limit derived from uac_mixer_unit_bmControls
> > > + if (desc->bLength < sizeof(*desc) + desc->bNrInPins
> + 2)
> > > + return 0; /* no bmControls -> skip */
> > > +
> > > mu_channels = get_cluster_channels_v3(state,
> > > uac3_mixer_unit_wClusterDescrID
> (desc));
> > > break;
> > > +
> > > + default:
> > > + break;
> > > }
> > >
> > > if (!mu_channels)
> > > --
> > > 2.22.1
> > >
> > >
> >
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
@ 2019-08-19 22:00 Hui Peng
2019-08-19 22:01 ` Hui Peng
0 siblings, 1 reply; 10+ messages in thread
From: Hui Peng @ 2019-08-19 22:00 UTC (permalink / raw)
To: security
Cc: Hui Peng, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
alsa-devel, linux-kernel
`uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
to get pointer to bmControls field. The current implementation of
`uac_mixer_unit_get_channels` does properly check the size of
uac_mixer_unit_descriptor descriptor and may allow OOB access
in `uac_mixer_unit_bmControls`.
Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
Signed-off-by: Hui Peng <benquike@gmail.com>
---
sound/usb/mixer.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b5927c3d5bc0..00e6274a63c3 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct mixer_build *state, unsigned int clust
static int uac_mixer_unit_get_channels(struct mixer_build *state,
struct uac_mixer_unit_descriptor *desc)
{
- int mu_channels;
+ int mu_channels = 0;
void *c;
- if (desc->bLength < sizeof(*desc))
- return -EINVAL;
if (!desc->bNrInPins)
return -EINVAL;
- if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
- return -EINVAL;
switch (state->mixer->protocol) {
case UAC_VERSION_1:
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
+ return 0;
+
+ mu_channels = uac_mixer_unit_bNrChannels(desc);
+ break;
+
case UAC_VERSION_2:
- default:
- if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
return 0; /* no bmControls -> skip */
+
mu_channels = uac_mixer_unit_bNrChannels(desc);
break;
case UAC_VERSION_3:
+ // limit derived from uac_mixer_unit_bmControls
+ if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
+ return 0; /* no bmControls -> skip */
+
mu_channels = get_cluster_channels_v3(state,
uac3_mixer_unit_wClusterDescrID(desc));
break;
+
+ default:
+ break;
}
if (!mu_channels)
--
2.22.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
2019-08-19 22:00 Hui Peng
@ 2019-08-19 22:01 ` Hui Peng
0 siblings, 0 replies; 10+ messages in thread
From: Hui Peng @ 2019-08-19 22:01 UTC (permalink / raw)
To: security; +Cc: linux-kernel, Mathias Payer, Takashi Iwai, alsa-devel
Sorry, this is the wrong patch. I will send it again.
On Mon, Aug 19, 2019 at 6:00 PM Hui Peng <benquike@gmail.com> wrote:
> `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> to get pointer to bmControls field. The current implementation of
> `uac_mixer_unit_get_channels` does properly check the size of
> uac_mixer_unit_descriptor descriptor and may allow OOB access
> in `uac_mixer_unit_bmControls`.
>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> sound/usb/mixer.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index b5927c3d5bc0..00e6274a63c3 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> mixer_build *state, unsigned int clust
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
> struct uac_mixer_unit_descriptor
> *desc)
> {
> - int mu_channels;
> + int mu_channels = 0;
> void *c;
>
> - if (desc->bLength < sizeof(*desc))
> - return -EINVAL;
> if (!desc->bNrInPins)
> return -EINVAL;
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> - return -EINVAL;
>
> switch (state->mixer->protocol) {
> case UAC_VERSION_1:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> + return 0;
> +
> + mu_channels = uac_mixer_unit_bNrChannels(desc);
> + break;
> +
> case UAC_VERSION_2:
> - default:
> - if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> return 0; /* no bmControls -> skip */
> +
> mu_channels = uac_mixer_unit_bNrChannels(desc);
> break;
> case UAC_VERSION_3:
> + // limit derived from uac_mixer_unit_bmControls
> + if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> + return 0; /* no bmControls -> skip */
> +
> mu_channels = get_cluster_channels_v3(state,
> uac3_mixer_unit_wClusterDescrID(desc));
> break;
> +
> + default:
> + break;
> }
>
> if (!mu_channels)
> --
> 2.22.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-08-19 22:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-17 4:32 [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls Hui Peng
2019-08-17 6:55 ` Takashi Iwai
2019-08-17 6:55 ` Takashi Iwai
2019-08-17 15:57 ` Hui Peng
2019-08-17 16:18 ` Takashi Iwai
2019-08-17 16:47 ` Hui Peng
2019-08-17 17:59 ` Takashi Iwai
2019-08-17 17:59 ` Takashi Iwai
-- strict thread matches above, loose matches on Subject: below --
2019-08-19 22:00 Hui Peng
2019-08-19 22:01 ` Hui Peng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.