* [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads @ 2020-01-08 23:43 Laszlo Ersek 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-08 23:43 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Hao A Wu, Jian J Wang, Jiaxin Wu, Maciej Rabeda, Ray Ni, Siyuan Fu, Zhichao Gao Repo: https://github.com/lersek/edk2.git Branch: tweaks_for_large_http This series aims to improve HTTP(S) Boot experience with large (4GiB+) files. Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Siyuan Fu <siyuan.fu@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Thanks, Laszlo Laszlo Ersek (2): MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 ++++++++++++++++++++ NetworkPkg/HttpDxe/HttpImpl.c | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-) -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-08 23:43 [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek @ 2020-01-08 23:43 ` Laszlo Ersek 2020-01-09 3:08 ` Ni, Ray ` (3 more replies) 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek 2020-01-14 10:59 ` [edk2-devel] [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek 2 siblings, 4 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-08 23:43 UTC (permalink / raw) To: edk2-devel-groups-io; +Cc: Hao A Wu, Jian J Wang, Ray Ni, Zhichao Gao The LoadFile protocol can report such a large buffer size that we cannot allocate enough reserved pages for. This particularly affects HTTP(S) Boot, if the remote file is very large (for example, an ISO image). While the TianoCore wiki mentions this at <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-image-size>: > The maximum RAM disk image size depends on how much continuous reserved > memory block the platform could provide. it's hard to remember; so log a DEBUG_ERROR message when the allocation fails. This patch produces error messages such as: > UiApp:BmExpandLoadFile: failed to allocate reserved pages: > BufferSize=4501536768 > LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ > IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ > Dns(192.168.124.1)/ > Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" > FilePath="" (Manually rewrapped here for keeping PatchCheck.py happy.) Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c index 62c5b2dc94ab..540d169ec1a6 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( // FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); if (FileBuffer == NULL) { + DEBUG_CODE ( + EFI_DEVICE_PATH *LoadFilePath; + CHAR16 *LoadFileText; + CHAR16 *FileText; + + LoadFilePath = DevicePathFromHandle (LoadFileHandle); + if (LoadFilePath == NULL) { + LoadFileText = NULL; + } else { + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); + } + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); + + DEBUG (( + DEBUG_ERROR, + "%a:%a: failed to allocate reserved pages: " + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", + gEfiCallerBaseName, + __FUNCTION__, + (UINT64)BufferSize, + LoadFileText, + FileText + )); + + if (FileText != NULL) { + FreePool (FileText); + } + if (LoadFileText != NULL) { + FreePool (LoadFileText); + } + ); return NULL; } -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek @ 2020-01-09 3:08 ` Ni, Ray 2020-01-09 17:24 ` [edk2-devel] " Philippe Mathieu-Daudé ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Ni, Ray @ 2020-01-09 3:08 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-groups-io, Fu, Siyuan Cc: Wu, Hao A, Wang, Jian J, Gao, Zhichao + Siyuan who originally worked in the HTTP boot functionality. > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Thursday, January 9, 2020 7:43 AM > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > Subject: [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved > mem allocation failure > > The LoadFile protocol can report such a large buffer size that we cannot > allocate enough reserved pages for. This particularly affects HTTP(S) > Boot, if the remote file is very large (for example, an ISO image). > > While the TianoCore wiki mentions this at > <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram- > disk-image-size>: > > > The maximum RAM disk image size depends on how much continuous > reserved > > memory block the platform could provide. > > it's hard to remember; so log a DEBUG_ERROR message when the allocation > fails. > > This patch produces error messages such as: > > > UiApp:BmExpandLoadFile: failed to allocate reserved pages: > > BufferSize=4501536768 > > LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ > > IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ > > Dns(192.168.124.1)/ > > Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" > > FilePath="" > > (Manually rewrapped here for keeping PatchCheck.py happy.) > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 > ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index 62c5b2dc94ab..540d169ec1a6 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( > // > FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); > if (FileBuffer == NULL) { > + DEBUG_CODE ( > + EFI_DEVICE_PATH *LoadFilePath; > + CHAR16 *LoadFileText; > + CHAR16 *FileText; > + > + LoadFilePath = DevicePathFromHandle (LoadFileHandle); > + if (LoadFilePath == NULL) { > + LoadFileText = NULL; > + } else { > + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); > + } > + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); > + > + DEBUG (( > + DEBUG_ERROR, > + "%a:%a: failed to allocate reserved pages: " > + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + (UINT64)BufferSize, > + LoadFileText, > + FileText > + )); > + > + if (FileText != NULL) { > + FreePool (FileText); > + } > + if (LoadFileText != NULL) { > + FreePool (LoadFileText); > + } > + ); > return NULL; > } > > -- > 2.19.1.3.g30247aa5d201 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek 2020-01-09 3:08 ` Ni, Ray @ 2020-01-09 17:24 ` Philippe Mathieu-Daudé 2020-01-13 1:53 ` Siyuan, Fu 2020-01-14 6:55 ` Wu, Hao A 3 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-09 17:24 UTC (permalink / raw) To: devel, lersek; +Cc: Hao A Wu, Jian J Wang, Ray Ni, Zhichao Gao On 1/9/20 12:43 AM, Laszlo Ersek wrote: > The LoadFile protocol can report such a large buffer size that we cannot > allocate enough reserved pages for. This particularly affects HTTP(S) > Boot, if the remote file is very large (for example, an ISO image). > > While the TianoCore wiki mentions this at > <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-image-size>: > >> The maximum RAM disk image size depends on how much continuous reserved >> memory block the platform could provide. > > it's hard to remember; so log a DEBUG_ERROR message when the allocation > fails. > > This patch produces error messages such as: > >> UiApp:BmExpandLoadFile: failed to allocate reserved pages: >> BufferSize=4501536768 >> LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ >> IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ >> Dns(192.168.124.1)/ >> Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" >> FilePath="" > > (Manually rewrapped here for keeping PatchCheck.py happy.) > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index 62c5b2dc94ab..540d169ec1a6 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( > // > FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); > if (FileBuffer == NULL) { > + DEBUG_CODE ( > + EFI_DEVICE_PATH *LoadFilePath; > + CHAR16 *LoadFileText; > + CHAR16 *FileText; > + > + LoadFilePath = DevicePathFromHandle (LoadFileHandle); > + if (LoadFilePath == NULL) { > + LoadFileText = NULL; > + } else { > + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); > + } > + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); > + > + DEBUG (( > + DEBUG_ERROR, > + "%a:%a: failed to allocate reserved pages: " > + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + (UINT64)BufferSize, > + LoadFileText, > + FileText > + )); > + > + if (FileText != NULL) { > + FreePool (FileText); Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > + } > + if (LoadFileText != NULL) { > + FreePool (LoadFileText); > + } > + ); > return NULL; > } > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek 2020-01-09 3:08 ` Ni, Ray 2020-01-09 17:24 ` [edk2-devel] " Philippe Mathieu-Daudé @ 2020-01-13 1:53 ` Siyuan, Fu 2020-01-13 21:05 ` Laszlo Ersek 2020-01-14 6:55 ` Wu, Hao A 3 siblings, 1 reply; 13+ messages in thread From: Siyuan, Fu @ 2020-01-13 1:53 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Wu, Hao A, Wang, Jian J, Ni, Ray, Gao, Zhichao The change looks good to me. Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: 2020年1月9日 7:43 > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log > reserved mem allocation failure > > The LoadFile protocol can report such a large buffer size that we cannot > allocate enough reserved pages for. This particularly affects HTTP(S) > Boot, if the remote file is very large (for example, an ISO image). > > While the TianoCore wiki mentions this at > <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram- > disk-image-size>: > > > The maximum RAM disk image size depends on how much continuous > reserved > > memory block the platform could provide. > > it's hard to remember; so log a DEBUG_ERROR message when the allocation > fails. > > This patch produces error messages such as: > > > UiApp:BmExpandLoadFile: failed to allocate reserved pages: > > BufferSize=4501536768 > > LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ > > IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ > > Dns(192.168.124.1)/ > > Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" > > FilePath="" > > (Manually rewrapped here for keeping PatchCheck.py happy.) > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 > ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index 62c5b2dc94ab..540d169ec1a6 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( > // > FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); > if (FileBuffer == NULL) { > + DEBUG_CODE ( > + EFI_DEVICE_PATH *LoadFilePath; > + CHAR16 *LoadFileText; > + CHAR16 *FileText; > + > + LoadFilePath = DevicePathFromHandle (LoadFileHandle); > + if (LoadFilePath == NULL) { > + LoadFileText = NULL; > + } else { > + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); > + } > + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); > + > + DEBUG (( > + DEBUG_ERROR, > + "%a:%a: failed to allocate reserved pages: " > + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + (UINT64)BufferSize, > + LoadFileText, > + FileText > + )); > + > + if (FileText != NULL) { > + FreePool (FileText); > + } > + if (LoadFileText != NULL) { > + FreePool (LoadFileText); > + } > + ); > return NULL; > } > > -- > 2.19.1.3.g30247aa5d201 > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-13 1:53 ` Siyuan, Fu @ 2020-01-13 21:05 ` Laszlo Ersek 0 siblings, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-13 21:05 UTC (permalink / raw) To: devel, siyuan.fu, Wu, Hao A, Wang, Jian J; +Cc: Ni, Ray, Gao, Zhichao On 01/13/20 02:53, Siyuan, Fu wrote: > The change looks good to me. > > Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> Thank you, Siyuan! Hao, Jian, can one of you please give a formal Acked-by, based on Siyuan's and Phil's "Reviewed-by"s? (I'm asking because I'm not sure if you would be comfortable with me pushing this without MdeModulePkg "M" approval.) Thanks! Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo >> Ersek >> Sent: 2020年1月9日 7:43 >> To: edk2-devel-groups-io <devel@edk2.groups.io> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; >> Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> >> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log >> reserved mem allocation failure >> >> The LoadFile protocol can report such a large buffer size that we cannot >> allocate enough reserved pages for. This particularly affects HTTP(S) >> Boot, if the remote file is very large (for example, an ISO image). >> >> While the TianoCore wiki mentions this at >> <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram- >> disk-image-size>: >> >>> The maximum RAM disk image size depends on how much continuous >> reserved >>> memory block the platform could provide. >> >> it's hard to remember; so log a DEBUG_ERROR message when the allocation >> fails. >> >> This patch produces error messages such as: >> >>> UiApp:BmExpandLoadFile: failed to allocate reserved pages: >>> BufferSize=4501536768 >>> LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ >>> IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ >>> Dns(192.168.124.1)/ >>> Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" >>> FilePath="" >> >> (Manually rewrapped here for keeping PatchCheck.py happy.) >> >> Cc: Hao A Wu <hao.a.wu@intel.com> >> Cc: Jian J Wang <jian.j.wang@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Zhichao Gao <zhichao.gao@intel.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 >> ++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >> index 62c5b2dc94ab..540d169ec1a6 100644 >> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >> @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( >> // >> FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); >> if (FileBuffer == NULL) { >> + DEBUG_CODE ( >> + EFI_DEVICE_PATH *LoadFilePath; >> + CHAR16 *LoadFileText; >> + CHAR16 *FileText; >> + >> + LoadFilePath = DevicePathFromHandle (LoadFileHandle); >> + if (LoadFilePath == NULL) { >> + LoadFileText = NULL; >> + } else { >> + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); >> + } >> + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); >> + >> + DEBUG (( >> + DEBUG_ERROR, >> + "%a:%a: failed to allocate reserved pages: " >> + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", >> + gEfiCallerBaseName, >> + __FUNCTION__, >> + (UINT64)BufferSize, >> + LoadFileText, >> + FileText >> + )); >> + >> + if (FileText != NULL) { >> + FreePool (FileText); >> + } >> + if (LoadFileText != NULL) { >> + FreePool (LoadFileText); >> + } >> + ); >> return NULL; >> } >> >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> >> > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek ` (2 preceding siblings ...) 2020-01-13 1:53 ` Siyuan, Fu @ 2020-01-14 6:55 ` Wu, Hao A 3 siblings, 0 replies; 13+ messages in thread From: Wu, Hao A @ 2020-01-14 6:55 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Wang, Jian J, Ni, Ray, Gao, Zhichao > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Thursday, January 09, 2020 7:43 AM > To: edk2-devel-groups-io > Cc: Wu, Hao A; Wang, Jian J; Ni, Ray; Gao, Zhichao > Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log > reserved mem allocation failure > > The LoadFile protocol can report such a large buffer size that we cannot > allocate enough reserved pages for. This particularly affects HTTP(S) > Boot, if the remote file is very large (for example, an ISO image). > > While the TianoCore wiki mentions this at > <https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram- > disk-image-size>: > > > The maximum RAM disk image size depends on how much continuous > reserved > > memory block the platform could provide. > > it's hard to remember; so log a DEBUG_ERROR message when the allocation > fails. > > This patch produces error messages such as: > > > UiApp:BmExpandLoadFile: failed to allocate reserved pages: > > BufferSize=4501536768 > > LoadFile="PciRoot(0x0)/Pci(0x3,0x0)/MAC(5254001B103E,0x1)/ > > IPv4(0.0.0.0,TCP,DHCP,192.168.124.106,192.168.124.1,255.255.255.0)/ > > Dns(192.168.124.1)/ > > Uri(https://ipv4-server/RHEL-7.7-20190723.1-Server-x86_64-dvd1.iso)" > > FilePath="" Acked-by: Hao A Wu <hao.a.wu@intel.com> Best Regards, Hao Wu > > (Manually rewrapped here for keeping PatchCheck.py happy.) > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 > ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > index 62c5b2dc94ab..540d169ec1a6 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > @@ -1387,6 +1387,37 @@ BmExpandLoadFile ( > // > FileBuffer = AllocateReservedPages (EFI_SIZE_TO_PAGES (BufferSize)); > if (FileBuffer == NULL) { > + DEBUG_CODE ( > + EFI_DEVICE_PATH *LoadFilePath; > + CHAR16 *LoadFileText; > + CHAR16 *FileText; > + > + LoadFilePath = DevicePathFromHandle (LoadFileHandle); > + if (LoadFilePath == NULL) { > + LoadFileText = NULL; > + } else { > + LoadFileText = ConvertDevicePathToText (LoadFilePath, FALSE, FALSE); > + } > + FileText = ConvertDevicePathToText (FilePath, FALSE, FALSE); > + > + DEBUG (( > + DEBUG_ERROR, > + "%a:%a: failed to allocate reserved pages: " > + "BufferSize=%Lu LoadFile=\"%s\" FilePath=\"%s\"\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + (UINT64)BufferSize, > + LoadFileText, > + FileText > + )); > + > + if (FileText != NULL) { > + FreePool (FileText); > + } > + if (LoadFileText != NULL) { > + FreePool (LoadFileText); > + } > + ); > return NULL; > } > > -- > 2.19.1.3.g30247aa5d201 > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download 2020-01-08 23:43 [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek @ 2020-01-08 23:43 ` Laszlo Ersek 2020-01-09 13:29 ` [edk2-devel] " Philippe Mathieu-Daudé ` (2 more replies) 2020-01-14 10:59 ` [edk2-devel] [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek 2 siblings, 3 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-08 23:43 UTC (permalink / raw) To: edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu 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.wu@intel.com> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> Cc: Siyuan Fu <siyuan.fu@intel.com> Signed-off-by: Laszlo Ersek <lersek@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); -- 2.19.1.3.g30247aa5d201 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek @ 2020-01-09 13:29 ` Philippe Mathieu-Daudé 2020-01-10 1:01 ` Siyuan, Fu 2020-01-10 15:31 ` Maciej Rabeda 2 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-09 13:29 UTC (permalink / raw) To: devel, lersek; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu On 1/9/20 12:43 AM, 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. Thanks for the very detailed explanation. Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > > 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.wu@intel.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Laszlo Ersek <lersek@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); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek 2020-01-09 13:29 ` [edk2-devel] " Philippe Mathieu-Daudé @ 2020-01-10 1:01 ` Siyuan, Fu 2020-01-10 10:49 ` Laszlo Ersek 2020-01-10 15:31 ` Maciej Rabeda 2 siblings, 1 reply; 13+ messages in thread From: Siyuan, Fu @ 2020-01-10 1:01 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Wu, Jiaxin, Maciej Rabeda Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: 2020年1月9日 7:43 > To: edk2-devel-groups-io <devel@edk2.groups.io> > Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda > <maciej.rabeda@linux.intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > Subject: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation > in HTTPS download > > 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.wu@intel.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Laszlo Ersek <lersek@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); > > -- > 2.19.1.3.g30247aa5d201 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download 2020-01-10 1:01 ` Siyuan, Fu @ 2020-01-10 10:49 ` Laszlo Ersek 0 siblings, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-10 10:49 UTC (permalink / raw) To: devel, siyuan.fu; +Cc: Wu, Jiaxin, Maciej Rabeda On 01/10/20 02:01, Siyuan, Fu wrote: > Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> Thanks, Siyuan! Can you please review patch#1 too in this series? (See Ray's request: <https://edk2.groups.io/g/devel/message/53044>.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek 2020-01-09 13:29 ` [edk2-devel] " Philippe Mathieu-Daudé 2020-01-10 1:01 ` Siyuan, Fu @ 2020-01-10 15:31 ` Maciej Rabeda 2 siblings, 0 replies; 13+ messages in thread From: Maciej Rabeda @ 2020-01-10 15:31 UTC (permalink / raw) To: devel, lersek; +Cc: Jiaxin Wu, Siyuan Fu Thanks for a very thorough explanation! Reviewed-by: Maciej Rabeda <maciej.rabeda@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.wu@intel.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Laszlo Ersek <lersek@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); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads 2020-01-08 23:43 [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek @ 2020-01-14 10:59 ` Laszlo Ersek 2 siblings, 0 replies; 13+ messages in thread From: Laszlo Ersek @ 2020-01-14 10:59 UTC (permalink / raw) To: edk2-devel-groups-io Cc: Hao A Wu, Jian J Wang, Jiaxin Wu, Maciej Rabeda, Ray Ni, Siyuan Fu, Zhichao Gao On 01/09/20 00:43, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: tweaks_for_large_http > > This series aims to improve HTTP(S) Boot experience with large (4GiB+) > files. > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Thanks, > Laszlo > > Laszlo Ersek (2): > MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure > NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download > > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 31 ++++++++++++++++++++ > NetworkPkg/HttpDxe/HttpImpl.c | 2 +- > 2 files changed, 32 insertions(+), 1 deletion(-) > Merged: commit range b112ec225f1c..4cca7923992a. Thanks! Laszlo ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-01-14 10:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-01-08 23:43 [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek 2020-01-08 23:43 ` [PATCH 1/2] MdeModulePkg/UefiBootManagerLib: log reserved mem allocation failure Laszlo Ersek 2020-01-09 3:08 ` Ni, Ray 2020-01-09 17:24 ` [edk2-devel] " Philippe Mathieu-Daudé 2020-01-13 1:53 ` Siyuan, Fu 2020-01-13 21:05 ` Laszlo Ersek 2020-01-14 6:55 ` Wu, Hao A 2020-01-08 23:43 ` [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download Laszlo Ersek 2020-01-09 13:29 ` [edk2-devel] " Philippe Mathieu-Daudé 2020-01-10 1:01 ` Siyuan, Fu 2020-01-10 10:49 ` Laszlo Ersek 2020-01-10 15:31 ` Maciej Rabeda 2020-01-14 10:59 ` [edk2-devel] [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox