From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.20; helo=mga02.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 527B921B02822 for ; Mon, 15 Oct 2018 20:39:14 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Oct 2018 20:39:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,387,1534834800"; d="scan'208";a="100559833" Received: from shzintpr02.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.160]) by orsmga002.jf.intel.com with ESMTP; 15 Oct 2018 20:39:13 -0700 To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Jiewen Yao , star.zeng@intel.com References: <20181015063833.61304-1-ruiyu.ni@intel.com> <20181015063833.61304-4-ruiyu.ni@intel.com> From: "Zeng, Star" Message-ID: Date: Tue, 16 Oct 2018 11:38:42 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181015063833.61304-4-ruiyu.ni@intel.com> Subject: Re: [PATCH 03/11] MdeModulePkg/UsbBus: Fix out-of-bound read access to descriptors X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Oct 2018 03:39:14 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2018/10/15 14:38, Ruiyu Ni wrote: > Today's implementation reads the Type/Length field in the USB > descriptors data without checking whether the offset to read is > beyond the data boundary. > > The patch fixes this issue. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Star Zeng > Cc: Jiewen Yao Ray, Thanks for the patch. I have two minor comments. You can take them as you wish as I have no strong opinion for them. :) Reviewed-by: Star Zeng > --- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 50 ++++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 12 deletions(-) > > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > index 061e4622e8..a93060deea 100644 > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c > @@ -177,6 +177,17 @@ UsbCreateDesc ( > DescLen = sizeof (EFI_USB_ENDPOINT_DESCRIPTOR); > CtrlLen = sizeof (USB_ENDPOINT_DESC); > break; > + > + default: > + ASSERT (FALSE); > + return NULL; > + } > + > + // > + // Total length is too small that cannot hold the single descriptor header plus data. > + // > + if (Len <= sizeof (USB_DESC_HEAD)) { Add debug message here like others below for error cases? > + return NULL; > } > > // > @@ -184,24 +195,39 @@ UsbCreateDesc ( > // format. Skip the descriptor that isn't of this Type > // > Offset = 0; > - Head = (USB_DESC_HEAD*)DescBuf; > + Head = (USB_DESC_HEAD *)DescBuf; > + while (Offset < Len - sizeof (USB_DESC_HEAD)) { > + // > + // Above condition make sure Head->Len and Head->Type are safe to access > + // > + Head = (USB_DESC_HEAD *)&DescBuf[Offset]; > > - while ((Offset < Len) && (Head->Type != Type)) { > - Offset += Head->Len; > - if (Len <= Offset) { > - DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Beyond boundary!\n")); > + if (Head->Len == 0) { > + DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = 0!\n")); > return NULL; > } > - Head = (USB_DESC_HEAD*)(DescBuf + Offset); > - if (Head->Len == 0) { > - DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = 0!\n")); > + > + // > + // Make sure no overflow when adding Head->Len to Offset. > + // > + if (Head->Len > MAX_UINTN - Offset) { > + DEBUG ((DEBUG_ERROR, "UsbCreateDesc: met mal-format descriptor, Head->Len = %d!\n", Head->Len)); > return NULL; > } > + > + Offset += Head->Len; > + > + if (Head->Type == Type) { > + break; > + } > } > > - if ((Len <= Offset) || (Len < Offset + Head->Len) || > - (Head->Type != Type) || (Head->Len < DescLen)) { > - DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor\n")); > + // > + // Head->Len is invalid resulting data beyond boundary, or > + // Descriptor cannot be found: No such type. > + // > + if ((Len < Offset) || (Head->Type != Type) || (Head->Len < DescLen)) { > + DEBUG (( EFI_D_ERROR, "UsbCreateDesc: met mal-format descriptor. Offset/Len = %d/%d, Header(T/L) = %d/%d\n", Offset, Len, Head->Type, Head->Len)); How about splitting the condition check and debug message to two? :) if (Len < Offset) { // It is boundary check. if ((Head->Type != Type) || (Head->Len < DescLen)) { // It is content check. Thanks, Star > return NULL; > } > > @@ -212,7 +238,7 @@ UsbCreateDesc ( > > CopyMem (Desc, Head, (UINTN) DescLen); > > - *Consumed = Offset + Head->Len; > + *Consumed = Offset; > > return Desc; > } >