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 93FE221162CCF for ; Mon, 15 Oct 2018 20:20:04 -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:20:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,387,1534834800"; d="scan'208";a="100555435" Received: from shzintpr04.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.101]) by orsmga002.jf.intel.com with ESMTP; 15 Oct 2018 20:20:03 -0700 To: Ruiyu Ni , edk2-devel@lists.01.org References: <20181015063833.61304-1-ruiyu.ni@intel.com> <20181015063833.61304-3-ruiyu.ni@intel.com> Cc: star.zeng@intel.com From: "Zeng, Star" Message-ID: <63407d7d-cf64-e652-a77b-63ddd6758937@intel.com> Date: Tue, 16 Oct 2018 11:19:32 +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-3-ruiyu.ni@intel.com> Subject: Re: [PATCH 02/11] MdeModulePkg/UsbMass: Fix integer overflow when BlockSize is 1 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:20:04 -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: > UsbBootReadWriteBlocks() and UsbBootReadWriteBlocks16() use a UINT16 > local variable to hold the value of > USB_BOOT_MAX_CARRY_SIZE (=0x10000) / BlockSize. > When BlockSize is 1, the UINT16 local variable is set to 0x10000 Ray, Thanks for the patch. Is it possible that the BlockSize == 0 or > USB_BOOT_MAX_CARRY_SIZE? Thanks, Star > but the high-16 bits are truncated resulting the final value be 0. > > It causes the while-loop in the two functions accesses 0 block in > each loop, resulting the loop never ends. > > The patch fixes the two functions to make sure no integer overflow > happens. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Star Zeng > Cc: Steven Shi > --- > .../Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c | 27 +++++++++++----------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > index 581571ab45..37fbeedbeb 100644 > --- a/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > +++ b/MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassBoot.c > @@ -815,14 +815,14 @@ UsbBootReadWriteBlocks ( > { > USB_BOOT_READ_WRITE_10_CMD Cmd; > EFI_STATUS Status; > - UINT16 Count; > - UINT16 CountMax; > + UINT32 Count; > + UINT32 CountMax; > UINT32 BlockSize; > UINT32 ByteSize; > UINT32 Timeout; > > BlockSize = UsbMass->BlockIoMedia.BlockSize; > - CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize); > + CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize; > Status = EFI_SUCCESS; > > while (TotalBlock > 0) { > @@ -831,8 +831,9 @@ UsbBootReadWriteBlocks ( > // on the device. We must split the total block because the READ10 > // command only has 16 bit transfer length (in the unit of block). > // > - Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax); > - ByteSize = (UINT32)Count * BlockSize; > + Count = (UINT32)MIN (TotalBlock, CountMax); > + Count = MIN (MAX_UINT16, Count); > + ByteSize = Count * BlockSize; > > // > // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1] > @@ -847,7 +848,7 @@ UsbBootReadWriteBlocks ( > Cmd.OpCode = Write ? USB_BOOT_WRITE10_OPCODE : USB_BOOT_READ10_OPCODE; > Cmd.Lun = (UINT8) (USB_BOOT_LUN (UsbMass->Lun)); > WriteUnaligned32 ((UINT32 *) Cmd.Lba, SwapBytes32 (Lba)); > - WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 (Count)); > + WriteUnaligned16 ((UINT16 *) Cmd.TransferLen, SwapBytes16 ((UINT16)Count)); > > Status = UsbBootExecCmdWithRetry ( > UsbMass, > @@ -867,7 +868,7 @@ UsbBootReadWriteBlocks ( > Lba, Count > )); > Lba += Count; > - Buffer += Count * BlockSize; > + Buffer += ByteSize; > TotalBlock -= Count; > } > > @@ -897,22 +898,22 @@ UsbBootReadWriteBlocks16 ( > { > UINT8 Cmd[16]; > EFI_STATUS Status; > - UINT16 Count; > - UINT16 CountMax; > + UINT32 Count; > + UINT32 CountMax; > UINT32 BlockSize; > UINT32 ByteSize; > UINT32 Timeout; > > BlockSize = UsbMass->BlockIoMedia.BlockSize; > - CountMax = (UINT16)(USB_BOOT_MAX_CARRY_SIZE / BlockSize); > + CountMax = USB_BOOT_MAX_CARRY_SIZE / BlockSize; > Status = EFI_SUCCESS; > > while (TotalBlock > 0) { > // > // Split the total blocks into smaller pieces. > // > - Count = (UINT16)((TotalBlock < CountMax) ? TotalBlock : CountMax); > - ByteSize = (UINT32)Count * BlockSize; > + Count = (UINT32)MIN (TotalBlock, CountMax); > + ByteSize = Count * BlockSize; > > // > // USB command's upper limit timeout is 5s. [USB2.0-9.2.6.1] > @@ -947,7 +948,7 @@ UsbBootReadWriteBlocks16 ( > Lba, Count > )); > Lba += Count; > - Buffer += Count * BlockSize; > + Buffer += ByteSize; > TotalBlock -= Count; > } > >