Thanks for a very thorough explanation! Reviewed-by: Maciej Rabeda <maciej.rab...@linux.intel.com>
On 09-Jan-20 00:43, Laszlo Ersek wrote:
When downloading over TLS, each TLS message ("APP packet") is returned as a (decrypted) fragment table by EFI_TLS_PROTOCOL.ProcessPacket(). The TlsProcessMessage() function in "NetworkPkg/HttpDxe/HttpsSupport.c" linearizes the fragment table into a single contiguous data block. The resultant flat data block contains both TLS headers and data. The HttpsReceive() function parses the actual application data -- in this case: decrypted HTTP data -- out of the flattened TLS data block, peeling off the TLS headers. The HttpResponseWorker() function in "NetworkPkg/HttpDxe/HttpImpl.c" propagates this HTTP data outwards, implementing the EFI_HTTP_PROTOCOL.Response() function. Now consider the following documentation for EFI_HTTP_PROTOCOL.Response(), quoted from "MdePkg/Include/Protocol/Http.h":It is the responsibility of the caller to allocate a buffer for Body and specify the size in BodyLength. If the remote host provides a response that contains a content body, up to BodyLength bytes will be copied from the receive buffer into Body and BodyLength will be updated with the amount of bytes received and copied to Body. This allows the client to download a large file in chunks instead of into one contiguous block of memory.Note that, if the caller-allocated buffer is larger than the server-provided chunk, then the transfer length is limited by the latter. This is in fact the dominant case when downloading a huge file (for which UefiBootManagerLib allocated a huge contiguous RAM Disk buffer) in small TLS messages. For adjusting BodyLength as described above -- i.e., to the application data chunk that has been extracted from the TLS message --, the HttpResponseWorker() function employs the following assignment: HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength); The (UINT32) cast is motivated by the MIN() requirement -- in "MdePkg/Include/Base.h" -- that both arguments be of the same type. "Fragment.Len" (NET_FRAGMENT.Len) has type UINT32, and "HttpMsg->BodyLength" (EFI_HTTP_MESSAGE.BodyLength) has type UINTN. Therefore a cast is indeed necessary. Unfortunately, the cast is done in the wrong direction. Consider the following circumstances: - "Fragment.Len" happens to be consistently 16KiB, dictated by the HTTPS Server's TLS stack, - the size of the file to download is 4GiB + N*16KiB, where N is a positive integer. As the download progresses, each received 16KiB application data chunk brings the *next* input value of BodyLength closer down to 4GiB. The cast in MIN() always masks off the high-order bits from the input value of BodyLength, but this is no problem because the low-order bits are nonzero, therefore the MIN() always permits progress. However, once BodyLength reaches 4GiB exactly on input, the MIN() invocation produces a zero value. HttpResponseWorker() adjusts the output value of BodyLength to zero, and then passes it to HttpParseMessageBody(). HttpParseMessageBody() (in "NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c") rejects the zero BodyLength with EFI_INVALID_PARAMETER, which is fully propagated outwards, and aborts the HTTPS download. HttpBootDxe writes the message "Error: Unexpected network error" to the UEFI console. For example, a file with size (4GiB + 197MiB) terminates after downloading just 197MiB. Invert the direction of the cast: widen "Fragment.Len" to UINTN. Cc: Jiaxin Wu <jiaxin...@intel.com> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> Cc: Siyuan Fu <siyuan...@intel.com> Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- NetworkPkg/HttpDxe/HttpImpl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c index 6b877314bd57..1acbb60d1014 100644 --- a/NetworkPkg/HttpDxe/HttpImpl.c +++ b/NetworkPkg/HttpDxe/HttpImpl.c @@ -1348,7 +1348,7 @@ HttpResponseWorker ( // // Process the received the body packet. // - HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength); + HttpMsg->BodyLength = MIN ((UINTN) Fragment.Len, HttpMsg->BodyLength);CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53162): https://edk2.groups.io/g/devel/message/53162 Mute This Topic: https://groups.io/mt/69550085/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
