Hi, Songpeng The change is ok with me while I have one comment for the original AllocateZeroPool() in these places. Since there will be always a CopyMem() to fill up data content to the new allocated buffer, there is no need to use AllocateZeroPool(), just AllocatePool() and adding null terminator should be enough. This will save the unnecessary ZeroMem() time cost for better performance. Thanks.
BestRegards Fu Siyuan > -----Original Message----- > From: Li, Songpeng > Sent: Tuesday, September 25, 2018 11:29 AM > To: [email protected] > Cc: Fu, Siyuan <[email protected]>; Wu, Jiaxin <[email protected]> > Subject: [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot. > > The input param String of AsciiStrStr() requires a pointer to > Null-terminated string, however in HttpTcpReceiveHeader() and > HttpUtilitiesParse(), the Buffersize before AllocateZeroPool() > is equal to the size of TCP header, after the CopyMem(), it > might not end with Null-terminator. It might cause memory > access overflow. > > Cc: Fu Siyuan <[email protected]> > Cc: Wu Jiaxin <[email protected]> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1204 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Songpeng Li <[email protected]> > --- > NetworkPkg/HttpDxe/HttpProto.c | 4 ++-- > NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c | 4 +++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/NetworkPkg/HttpDxe/HttpProto.c > b/NetworkPkg/HttpDxe/HttpProto.c > index 94f89f5665..c729f76eff 100644 > --- a/NetworkPkg/HttpDxe/HttpProto.c > +++ b/NetworkPkg/HttpDxe/HttpProto.c > @@ -1917,7 +1917,7 @@ HttpTcpReceiveHeader ( > // Append the response string. > // > *BufferSize = *SizeofHeaders + Fragment.Len; > - Buffer = AllocateZeroPool (*BufferSize); > + Buffer = AllocateZeroPool (*BufferSize + 1); > if (Buffer == NULL) { > Status = EFI_OUT_OF_RESOURCES; > return Status; > @@ -2016,7 +2016,7 @@ HttpTcpReceiveHeader ( > // Append the response string. > // > *BufferSize = *SizeofHeaders + Fragment.Len; > - Buffer = AllocateZeroPool (*BufferSize); > + Buffer = AllocateZeroPool (*BufferSize + 1); > if (Buffer == NULL) { > Status = EFI_OUT_OF_RESOURCES; > return Status; > diff --git a/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c > b/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c > index a9a1c7c586..2292b52537 100644 > --- a/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c > +++ b/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c > @@ -298,6 +298,7 @@ HttpUtilitiesParse ( > CHAR8 *FieldName; > CHAR8 *FieldValue; > UINTN Index; > + UINTN HttpBufferSize; > > Status = EFI_SUCCESS; > TempHttpMessage = NULL; > @@ -311,7 +312,8 @@ HttpUtilitiesParse ( > return EFI_INVALID_PARAMETER; > } > > - TempHttpMessage = AllocateZeroPool (HttpMessageSize); > + HttpBufferSize = HttpMessageSize + 1; > + TempHttpMessage = AllocateZeroPool (HttpBufferSize); > if (TempHttpMessage == NULL) { > return EFI_OUT_OF_RESOURCES; > } > -- > 2.18.0.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
