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_bmControlsif (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_bmControlsif (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_bmControlsif (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