* [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. @ 2020-02-27 11:02 Maciej Rabeda 2020-02-27 13:14 ` [edk2-devel] " Laszlo Ersek 2020-03-25 11:34 ` Laszlo Ersek 0 siblings, 2 replies; 14+ messages in thread From: Maciej Rabeda @ 2020-02-27 11:02 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Zhichao Gao REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 'ping' command's receive flow utilizes a single Rx token which it attempts to reuse before recycling the previously received packet. This causes a situation where under ICMP traffic, Ping6OnEchoReplyReceived() function will receive an already recycled packet with EFI_SUCCESS token status and finally dereference invalid pointers from RxData structure. Cc: Ray Ni <ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> --- ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c index 23567fa2c1bb..a3fa32515192 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( ON_EXIT: + // + // Recycle the packet before reusing RxToken + // + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); + if (Private->RxCount < Private->SendNum) { // // Continue to receive icmp echo reply packets. @@ -632,10 +637,6 @@ ON_EXIT: // Private->Status = EFI_SUCCESS; } - // - // Singal to recycle the each rxdata here, not at the end of process. - // - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); } /** -- 2.24.0.windows.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-27 11:02 [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow Maciej Rabeda @ 2020-02-27 13:14 ` Laszlo Ersek 2020-02-27 17:40 ` Laszlo Ersek 2020-03-25 11:34 ` Laszlo Ersek 1 sibling, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2020-02-27 13:14 UTC (permalink / raw) To: devel, maciej.rabeda, Liming Gao, Michael Kinney, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ray Ni, Zhichao Gao, nicholas.armour (+Liming and stewards; CC Nick) On 02/27/20 12:02, Maciej Rabeda wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > > 'ping' command's receive flow utilizes a single Rx token which it > attempts to reuse before recycling the previously received packet. > This causes a situation where under ICMP traffic, > Ping6OnEchoReplyReceived() function will receive an already > recycled packet with EFI_SUCCESS token status and finally > dereference invalid pointers from RxData structure. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > --- > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > index 23567fa2c1bb..a3fa32515192 100644 > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > > ON_EXIT: > > + // > + // Recycle the packet before reusing RxToken > + // > + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > + > if (Private->RxCount < Private->SendNum) { > // > // Continue to receive icmp echo reply packets. > @@ -632,10 +637,6 @@ ON_EXIT: > // > Private->Status = EFI_SUCCESS; > } > - // > - // Singal to recycle the each rxdata here, not at the end of process. > - // > - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > } > > /** > (1) This patch proposes to fix one of the BZs (2032) that fall under CVE-2019-14559 (joint tracker: 2550). Consequently: (1a) Do we want to include this in the upcoming stable tag? If so, we might want to extend the hard feature freeze by a few days. (1b) Please append the string " (CVE-2019-14559)" -- note the separating space! -- to the subject line. (2) However: I remember from an earlier Bugzilla entry (can't tell off-hand, which one, sorry) that ShellPkg issues are *never* considered CVE-worthy, because the shell is not considered a "production element" of the UEFI boot path. TianoCore#2032 was originally filed for NetworkPkg, and indeed that seemed to justify the CVE assignment. However, now that Nick's and Maciej's analysis shows that NetworkPkg is unaffected (and we know, per above, that ShellPkg is not CVE-worthy), should we rather *remove* this BZ from the CVE-2019-14559 umbrella? Because, in that case, modifying the subject line on the patch is not necessary; and more importantly, we might not even want to put this into edk2-stable202002. (It's still a bugfix, but may not be important enough.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-27 13:14 ` [edk2-devel] " Laszlo Ersek @ 2020-02-27 17:40 ` Laszlo Ersek 2020-02-28 2:59 ` Liming Gao 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2020-02-27 17:40 UTC (permalink / raw) To: devel, maciej.rabeda, Liming Gao, Michael Kinney, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ray Ni, Zhichao Gao, nicholas.armour On 02/27/20 14:14, Laszlo Ersek wrote: > (+Liming and stewards; CC Nick) > > On 02/27/20 12:02, Maciej Rabeda wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 >> >> 'ping' command's receive flow utilizes a single Rx token which it >> attempts to reuse before recycling the previously received packet. >> This causes a situation where under ICMP traffic, >> Ping6OnEchoReplyReceived() function will receive an already >> recycled packet with EFI_SUCCESS token status and finally >> dereference invalid pointers from RxData structure. >> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Zhichao Gao <zhichao.gao@intel.com> >> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >> --- >> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >> index 23567fa2c1bb..a3fa32515192 100644 >> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( >> >> ON_EXIT: >> >> + // >> + // Recycle the packet before reusing RxToken >> + // >> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >> + >> if (Private->RxCount < Private->SendNum) { >> // >> // Continue to receive icmp echo reply packets. >> @@ -632,10 +637,6 @@ ON_EXIT: >> // >> Private->Status = EFI_SUCCESS; >> } >> - // >> - // Singal to recycle the each rxdata here, not at the end of process. >> - // >> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >> } >> >> /** >> > > (1) This patch proposes to fix one of the BZs (2032) that fall under > CVE-2019-14559 (joint tracker: 2550). > > Consequently: > > (1a) Do we want to include this in the upcoming stable tag? > > If so, we might want to extend the hard feature freeze by a few days. > > (1b) Please append the string " (CVE-2019-14559)" -- note the separating > space! -- to the subject line. > > (2) However: I remember from an earlier Bugzilla entry (can't tell > off-hand, which one, sorry) that ShellPkg issues are *never* considered > CVE-worthy, because the shell is not considered a "production element" > of the UEFI boot path. I misremembered -- there is indeed a comment like that, in the TianoCore bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which has since been split off to the edk2-libc project): https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 StdLib is supposed to be used only by applications in shell, all of which are meant for debug, diagnosis and/or test purpose, not for product UEFI BIOS. Any issue in it will not be taken as security issue but just normal bug. Sorry about causing confusion. So, the ShellPkg maintainers should decide what to do about this bug (keep it under the CVE scope vs. exclude it from the CVE scope; and then, propose it for the stable tag or merge it afterwards). One data point: the bug appears to go back to the inception of the Ping command, in historical commit 68fb05272b45 ("Add Network1 profile.", 2011-03-25). It's not a new bug, it seems. Thanks Laszlo > > TianoCore#2032 was originally filed for NetworkPkg, and indeed that > seemed to justify the CVE assignment. However, now that Nick's and > Maciej's analysis shows that NetworkPkg is unaffected (and we know, per > above, that ShellPkg is not CVE-worthy), should we rather *remove* this > BZ from the CVE-2019-14559 umbrella? > > Because, in that case, modifying the subject line on the patch is not > necessary; and more importantly, we might not even want to put this into > edk2-stable202002. (It's still a bugfix, but may not be important enough.) > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-27 17:40 ` Laszlo Ersek @ 2020-02-28 2:59 ` Liming Gao 2020-02-28 11:41 ` Maciej Rabeda 0 siblings, 1 reply; 14+ messages in thread From: Liming Gao @ 2020-02-28 2:59 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, maciej.rabeda@linux.intel.com, Kinney, Michael D, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ni, Ray, Gao, Zhichao, Armour, Nicholas, Fu, Siyuan, Wu, Jiaxin Also include NetworkPkg reviewer to collect the feedback for this change. Thanks Liming > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Friday, February 28, 2020 1:40 AM > To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com> > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > > On 02/27/20 14:14, Laszlo Ersek wrote: > > (+Liming and stewards; CC Nick) > > > > On 02/27/20 12:02, Maciej Rabeda wrote: > >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > >> > >> 'ping' command's receive flow utilizes a single Rx token which it > >> attempts to reuse before recycling the previously received packet. > >> This causes a situation where under ICMP traffic, > >> Ping6OnEchoReplyReceived() function will receive an already > >> recycled packet with EFI_SUCCESS token status and finally > >> dereference invalid pointers from RxData structure. > >> > >> Cc: Ray Ni <ray.ni@intel.com> > >> Cc: Zhichao Gao <zhichao.gao@intel.com> > >> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > >> --- > >> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >> index 23567fa2c1bb..a3fa32515192 100644 > >> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > >> > >> ON_EXIT: > >> > >> + // > >> + // Recycle the packet before reusing RxToken > >> + // > >> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >> + > >> if (Private->RxCount < Private->SendNum) { > >> // > >> // Continue to receive icmp echo reply packets. > >> @@ -632,10 +637,6 @@ ON_EXIT: > >> // > >> Private->Status = EFI_SUCCESS; > >> } > >> - // > >> - // Singal to recycle the each rxdata here, not at the end of process. > >> - // > >> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >> } > >> > >> /** > >> > > > > (1) This patch proposes to fix one of the BZs (2032) that fall under > > CVE-2019-14559 (joint tracker: 2550). > > > > Consequently: > > > > (1a) Do we want to include this in the upcoming stable tag? > > > > If so, we might want to extend the hard feature freeze by a few days. > > > > (1b) Please append the string " (CVE-2019-14559)" -- note the separating > > space! -- to the subject line. > > > > (2) However: I remember from an earlier Bugzilla entry (can't tell > > off-hand, which one, sorry) that ShellPkg issues are *never* considered > > CVE-worthy, because the shell is not considered a "production element" > > of the UEFI boot path. > > I misremembered -- there is indeed a comment like that, in the TianoCore > bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which > has since been split off to the edk2-libc project): > > https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 > > StdLib is supposed to be used only by applications in shell, all of > which are meant for debug, diagnosis and/or test purpose, not for > product UEFI BIOS. Any issue in it will not be taken as security > issue but just normal bug. > > Sorry about causing confusion. So, the ShellPkg maintainers should > decide what to do about this bug (keep it under the CVE scope vs. > exclude it from the CVE scope; and then, propose it for the stable tag > or merge it afterwards). > > One data point: the bug appears to go back to the inception of the Ping > command, in historical commit 68fb05272b45 ("Add Network1 profile.", > 2011-03-25). It's not a new bug, it seems. > > Thanks > Laszlo > > > > > TianoCore#2032 was originally filed for NetworkPkg, and indeed that > > seemed to justify the CVE assignment. However, now that Nick's and > > Maciej's analysis shows that NetworkPkg is unaffected (and we know, per > > above, that ShellPkg is not CVE-worthy), should we rather *remove* this > > BZ from the CVE-2019-14559 umbrella? > > > > Because, in that case, modifying the subject line on the patch is not > > necessary; and more importantly, we might not even want to put this into > > edk2-stable202002. (It's still a bugfix, but may not be important enough.) > > > > Thanks! > > Laszlo > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-28 2:59 ` Liming Gao @ 2020-02-28 11:41 ` Maciej Rabeda 2020-02-28 11:50 ` Liming Gao 0 siblings, 1 reply; 14+ messages in thread From: Maciej Rabeda @ 2020-02-28 11:41 UTC (permalink / raw) To: devel, liming.gao, Laszlo Ersek, Kinney, Michael D, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ni, Ray, Gao, Zhichao, Armour, Nicholas, Fu, Siyuan, Wu, Jiaxin Laszlo, Thanks for the detailed response on the patch. Always happy to learn about stuff from the past. Liming, I am currently the maintainer of NetworkPkg :) If you require additional feedback from Siyuan or/and Jiaxin, that's ok. Please let me know if any corrections to the patch (like CVE note) are required from your point of view. Thanks, Maciej On 28-Feb-20 03:59, Liming Gao wrote: > Also include NetworkPkg reviewer to collect the feedback for this change. > > Thanks > Liming >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Friday, February 28, 2020 1:40 AM >> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> >> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com> >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. >> >> On 02/27/20 14:14, Laszlo Ersek wrote: >>> (+Liming and stewards; CC Nick) >>> >>> On 02/27/20 12:02, Maciej Rabeda wrote: >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 >>>> >>>> 'ping' command's receive flow utilizes a single Rx token which it >>>> attempts to reuse before recycling the previously received packet. >>>> This causes a situation where under ICMP traffic, >>>> Ping6OnEchoReplyReceived() function will receive an already >>>> recycled packet with EFI_SUCCESS token status and finally >>>> dereference invalid pointers from RxData structure. >>>> >>>> Cc: Ray Ni <ray.ni@intel.com> >>>> Cc: Zhichao Gao <zhichao.gao@intel.com> >>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>>> --- >>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- >>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> index 23567fa2c1bb..a3fa32515192 100644 >>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( >>>> >>>> ON_EXIT: >>>> >>>> + // >>>> + // Recycle the packet before reusing RxToken >>>> + // >>>> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- >>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >>>> + >>>> if (Private->RxCount < Private->SendNum) { >>>> // >>>> // Continue to receive icmp echo reply packets. >>>> @@ -632,10 +637,6 @@ ON_EXIT: >>>> // >>>> Private->Status = EFI_SUCCESS; >>>> } >>>> - // >>>> - // Singal to recycle the each rxdata here, not at the end of process. >>>> - // >>>> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- >>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >>>> } >>>> >>>> /** >>>> >>> (1) This patch proposes to fix one of the BZs (2032) that fall under >>> CVE-2019-14559 (joint tracker: 2550). >>> >>> Consequently: >>> >>> (1a) Do we want to include this in the upcoming stable tag? >>> >>> If so, we might want to extend the hard feature freeze by a few days. >>> >>> (1b) Please append the string " (CVE-2019-14559)" -- note the separating >>> space! -- to the subject line. >>> >>> (2) However: I remember from an earlier Bugzilla entry (can't tell >>> off-hand, which one, sorry) that ShellPkg issues are *never* considered >>> CVE-worthy, because the shell is not considered a "production element" >>> of the UEFI boot path. >> I misremembered -- there is indeed a comment like that, in the TianoCore >> bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which >> has since been split off to the edk2-libc project): >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 >> >> StdLib is supposed to be used only by applications in shell, all of >> which are meant for debug, diagnosis and/or test purpose, not for >> product UEFI BIOS. Any issue in it will not be taken as security >> issue but just normal bug. >> >> Sorry about causing confusion. So, the ShellPkg maintainers should >> decide what to do about this bug (keep it under the CVE scope vs. >> exclude it from the CVE scope; and then, propose it for the stable tag >> or merge it afterwards). >> >> One data point: the bug appears to go back to the inception of the Ping >> command, in historical commit 68fb05272b45 ("Add Network1 profile.", >> 2011-03-25). It's not a new bug, it seems. >> >> Thanks >> Laszlo >> >>> TianoCore#2032 was originally filed for NetworkPkg, and indeed that >>> seemed to justify the CVE assignment. However, now that Nick's and >>> Maciej's analysis shows that NetworkPkg is unaffected (and we know, per >>> above, that ShellPkg is not CVE-worthy), should we rather *remove* this >>> BZ from the CVE-2019-14559 umbrella? >>> >>> Because, in that case, modifying the subject line on the patch is not >>> necessary; and more importantly, we might not even want to put this into >>> edk2-stable202002. (It's still a bugfix, but may not be important enough.) >>> >>> Thanks! >>> Laszlo >>> > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-28 11:41 ` Maciej Rabeda @ 2020-02-28 11:50 ` Liming Gao 2020-02-28 12:35 ` Maciej Rabeda 0 siblings, 1 reply; 14+ messages in thread From: Liming Gao @ 2020-02-28 11:50 UTC (permalink / raw) To: Rabeda, Maciej, devel@edk2.groups.io, Laszlo Ersek, Kinney, Michael D, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ni, Ray, Gao, Zhichao, Armour, Nicholas, Fu, Siyuan, Wu, Jiaxin, Gao, Liming Maciej: I see you submit the patch. So, you are in the loop. I don't invite you again. 😊 Yes. I also want to get your opinion for this change. Do you think whether this fix is in CVE scope? If no, this change will be merged after this stable tag 202002. Is it OK to you? Thanks Liming > -----Original Message----- > From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> > Sent: Friday, February 28, 2020 7:42 PM > To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>; Fu, Siyuan > <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > > Laszlo, > > Thanks for the detailed response on the patch. Always happy to learn > about stuff from the past. > > Liming, > > I am currently the maintainer of NetworkPkg :) If you require additional > feedback from Siyuan or/and Jiaxin, that's ok. > Please let me know if any corrections to the patch (like CVE note) are > required from your point of view. > > Thanks, > Maciej > > On 28-Feb-20 03:59, Liming Gao wrote: > > Also include NetworkPkg reviewer to collect the feedback for this change. > > > > Thanks > > Liming > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Friday, February 28, 2020 1:40 AM > >> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D > >> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > >> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com> > >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > >> > >> On 02/27/20 14:14, Laszlo Ersek wrote: > >>> (+Liming and stewards; CC Nick) > >>> > >>> On 02/27/20 12:02, Maciej Rabeda wrote: > >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > >>>> > >>>> 'ping' command's receive flow utilizes a single Rx token which it > >>>> attempts to reuse before recycling the previously received packet. > >>>> This causes a situation where under ICMP traffic, > >>>> Ping6OnEchoReplyReceived() function will receive an already > >>>> recycled packet with EFI_SUCCESS token status and finally > >>>> dereference invalid pointers from RxData structure. > >>>> > >>>> Cc: Ray Ni <ray.ni@intel.com> > >>>> Cc: Zhichao Gao <zhichao.gao@intel.com> > >>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > >>>> --- > >>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > >>>> 1 file changed, 5 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>> index 23567fa2c1bb..a3fa32515192 100644 > >>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > >>>> > >>>> ON_EXIT: > >>>> > >>>> + // > >>>> + // Recycle the packet before reusing RxToken > >>>> + // > >>>> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >>>> + > >>>> if (Private->RxCount < Private->SendNum) { > >>>> // > >>>> // Continue to receive icmp echo reply packets. > >>>> @@ -632,10 +637,6 @@ ON_EXIT: > >>>> // > >>>> Private->Status = EFI_SUCCESS; > >>>> } > >>>> - // > >>>> - // Singal to recycle the each rxdata here, not at the end of process. > >>>> - // > >>>> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >>>> } > >>>> > >>>> /** > >>>> > >>> (1) This patch proposes to fix one of the BZs (2032) that fall under > >>> CVE-2019-14559 (joint tracker: 2550). > >>> > >>> Consequently: > >>> > >>> (1a) Do we want to include this in the upcoming stable tag? > >>> > >>> If so, we might want to extend the hard feature freeze by a few days. > >>> > >>> (1b) Please append the string " (CVE-2019-14559)" -- note the separating > >>> space! -- to the subject line. > >>> > >>> (2) However: I remember from an earlier Bugzilla entry (can't tell > >>> off-hand, which one, sorry) that ShellPkg issues are *never* considered > >>> CVE-worthy, because the shell is not considered a "production element" > >>> of the UEFI boot path. > >> I misremembered -- there is indeed a comment like that, in the TianoCore > >> bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which > >> has since been split off to the edk2-libc project): > >> > >> https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 > >> > >> StdLib is supposed to be used only by applications in shell, all of > >> which are meant for debug, diagnosis and/or test purpose, not for > >> product UEFI BIOS. Any issue in it will not be taken as security > >> issue but just normal bug. > >> > >> Sorry about causing confusion. So, the ShellPkg maintainers should > >> decide what to do about this bug (keep it under the CVE scope vs. > >> exclude it from the CVE scope; and then, propose it for the stable tag > >> or merge it afterwards). > >> > >> One data point: the bug appears to go back to the inception of the Ping > >> command, in historical commit 68fb05272b45 ("Add Network1 profile.", > >> 2011-03-25). It's not a new bug, it seems. > >> > >> Thanks > >> Laszlo > >> > >>> TianoCore#2032 was originally filed for NetworkPkg, and indeed that > >>> seemed to justify the CVE assignment. However, now that Nick's and > >>> Maciej's analysis shows that NetworkPkg is unaffected (and we know, per > >>> above, that ShellPkg is not CVE-worthy), should we rather *remove* this > >>> BZ from the CVE-2019-14559 umbrella? > >>> > >>> Because, in that case, modifying the subject line on the patch is not > >>> necessary; and more importantly, we might not even want to put this into > >>> edk2-stable202002. (It's still a bugfix, but may not be important enough.) > >>> > >>> Thanks! > >>> Laszlo > >>> > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-28 11:50 ` Liming Gao @ 2020-02-28 12:35 ` Maciej Rabeda 2020-03-02 13:43 ` Liming Gao 0 siblings, 1 reply; 14+ messages in thread From: Maciej Rabeda @ 2020-02-28 12:35 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Laszlo Ersek, Kinney, Michael D, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ni, Ray, Gao, Zhichao, Armour, Nicholas, Fu, Siyuan, Wu, Jiaxin Liming, I assume that CVE-2019-14559 relates to problems with network drivers within NetworkPkg. BZ 2032 and this patch fix address incorrect usage of IP4 protocol by ShellPkg 'ping' command (signalling packet recycling after triggering Ip4->Receive() on the same Rx token). This issue was reported in July 2019 and occurs by executing a specific fuzzing scenario (as described in BZ). As Laszlo has mentioned, it is not a new bug (introduced in 2011). Based on the above, I can advise moving this issue out of CVE scope (and from stable-202002). However, if the CVE should be treated as "overall problems with UEFI FW networking", then 'ping' command seems to be in CVE scope, despite the code residing in ShellPkg. Thanks, Maciej On 28-Feb-20 12:50, Gao, Liming wrote: > Maciej: > I see you submit the patch. So, you are in the loop. I don't invite you again. 😊 > > Yes. I also want to get your opinion for this change. Do you think whether this fix is in CVE scope? If no, this change will be merged after this stable tag 202002. Is it OK to you? > > Thanks > Liming >> -----Original Message----- >> From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> >> Sent: Friday, February 28, 2020 7:42 PM >> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D >> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> >> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>; Fu, Siyuan >> <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com> >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. >> >> Laszlo, >> >> Thanks for the detailed response on the patch. Always happy to learn >> about stuff from the past. >> >> Liming, >> >> I am currently the maintainer of NetworkPkg :) If you require additional >> feedback from Siyuan or/and Jiaxin, that's ok. >> Please let me know if any corrections to the patch (like CVE note) are >> required from your point of view. >> >> Thanks, >> Maciej >> >> On 28-Feb-20 03:59, Liming Gao wrote: >>> Also include NetworkPkg reviewer to collect the feedback for this change. >>> >>> Thanks >>> Liming >>>> -----Original Message----- >>>> From: Laszlo Ersek <lersek@redhat.com> >>>> Sent: Friday, February 28, 2020 1:40 AM >>>> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D >>>> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> >>>> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com> >>>> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. >>>> >>>> On 02/27/20 14:14, Laszlo Ersek wrote: >>>>> (+Liming and stewards; CC Nick) >>>>> >>>>> On 02/27/20 12:02, Maciej Rabeda wrote: >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 >>>>>> >>>>>> 'ping' command's receive flow utilizes a single Rx token which it >>>>>> attempts to reuse before recycling the previously received packet. >>>>>> This causes a situation where under ICMP traffic, >>>>>> Ping6OnEchoReplyReceived() function will receive an already >>>>>> recycled packet with EFI_SUCCESS token status and finally >>>>>> dereference invalid pointers from RxData structure. >>>>>> >>>>>> Cc: Ray Ni <ray.ni@intel.com> >>>>>> Cc: Zhichao Gao <zhichao.gao@intel.com> >>>>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>>>>> --- >>>>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- >>>>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>>>> index 23567fa2c1bb..a3fa32515192 100644 >>>>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>>>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( >>>>>> >>>>>> ON_EXIT: >>>>>> >>>>>> + // >>>>>> + // Recycle the packet before reusing RxToken >>>>>> + // >>>>>> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- >>>>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >>>>>> + >>>>>> if (Private->RxCount < Private->SendNum) { >>>>>> // >>>>>> // Continue to receive icmp echo reply packets. >>>>>> @@ -632,10 +637,6 @@ ON_EXIT: >>>>>> // >>>>>> Private->Status = EFI_SUCCESS; >>>>>> } >>>>>> - // >>>>>> - // Singal to recycle the each rxdata here, not at the end of process. >>>>>> - // >>>>>> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- >>>>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); >>>>>> } >>>>>> >>>>>> /** >>>>>> >>>>> (1) This patch proposes to fix one of the BZs (2032) that fall under >>>>> CVE-2019-14559 (joint tracker: 2550). >>>>> >>>>> Consequently: >>>>> >>>>> (1a) Do we want to include this in the upcoming stable tag? >>>>> >>>>> If so, we might want to extend the hard feature freeze by a few days. >>>>> >>>>> (1b) Please append the string " (CVE-2019-14559)" -- note the separating >>>>> space! -- to the subject line. >>>>> >>>>> (2) However: I remember from an earlier Bugzilla entry (can't tell >>>>> off-hand, which one, sorry) that ShellPkg issues are *never* considered >>>>> CVE-worthy, because the shell is not considered a "production element" >>>>> of the UEFI boot path. >>>> I misremembered -- there is indeed a comment like that, in the TianoCore >>>> bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which >>>> has since been split off to the edk2-libc project): >>>> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 >>>> >>>> StdLib is supposed to be used only by applications in shell, all of >>>> which are meant for debug, diagnosis and/or test purpose, not for >>>> product UEFI BIOS. Any issue in it will not be taken as security >>>> issue but just normal bug. >>>> >>>> Sorry about causing confusion. So, the ShellPkg maintainers should >>>> decide what to do about this bug (keep it under the CVE scope vs. >>>> exclude it from the CVE scope; and then, propose it for the stable tag >>>> or merge it afterwards). >>>> >>>> One data point: the bug appears to go back to the inception of the Ping >>>> command, in historical commit 68fb05272b45 ("Add Network1 profile.", >>>> 2011-03-25). It's not a new bug, it seems. >>>> >>>> Thanks >>>> Laszlo >>>> >>>>> TianoCore#2032 was originally filed for NetworkPkg, and indeed that >>>>> seemed to justify the CVE assignment. However, now that Nick's and >>>>> Maciej's analysis shows that NetworkPkg is unaffected (and we know, per >>>>> above, that ShellPkg is not CVE-worthy), should we rather *remove* this >>>>> BZ from the CVE-2019-14559 umbrella? >>>>> >>>>> Because, in that case, modifying the subject line on the patch is not >>>>> necessary; and more importantly, we might not even want to put this into >>>>> edk2-stable202002. (It's still a bugfix, but may not be important enough.) >>>>> >>>>> Thanks! >>>>> Laszlo >>>>> >>> >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-28 12:35 ` Maciej Rabeda @ 2020-03-02 13:43 ` Liming Gao 0 siblings, 0 replies; 14+ messages in thread From: Liming Gao @ 2020-03-02 13:43 UTC (permalink / raw) To: Rabeda, Maciej, devel@edk2.groups.io, Laszlo Ersek, Kinney, Michael D, Andrew Fish, Leif Lindholm (Linaro address) Cc: Ni, Ray, Gao, Zhichao, Armour, Nicholas, Fu, Siyuan, Wu, Jiaxin, Gao, Liming Maciej: Thanks for your analysis. I agree with you. This change is not necessary to catch edk2 stable tag 202002. Thanks Liming > -----Original Message----- > From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> > Sent: Friday, February 28, 2020 8:35 PM > To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>; Fu, Siyuan > <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > > Liming, > > I assume that CVE-2019-14559 relates to problems with network drivers > within NetworkPkg. > BZ 2032 and this patch fix address incorrect usage of IP4 protocol by > ShellPkg 'ping' command (signalling packet recycling after triggering > Ip4->Receive() on the same Rx token). > > This issue was reported in July 2019 and occurs by executing a specific > fuzzing scenario (as described in BZ). > As Laszlo has mentioned, it is not a new bug (introduced in 2011). > > Based on the above, I can advise moving this issue out of CVE scope (and > from stable-202002). > However, if the CVE should be treated as "overall problems with UEFI FW > networking", then 'ping' command seems to be in CVE scope, despite the > code residing in ShellPkg. > > Thanks, > Maciej > > On 28-Feb-20 12:50, Gao, Liming wrote: > > Maciej: > > I see you submit the patch. So, you are in the loop. I don't invite you again. 😊 > > > > Yes. I also want to get your opinion for this change. Do you think whether this fix is in CVE scope? If no, this change will be merged > after this stable tag 202002. Is it OK to you? > > > > Thanks > > Liming > >> -----Original Message----- > >> From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> > >> Sent: Friday, February 28, 2020 7:42 PM > >> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D > >> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > >> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>; Fu, Siyuan > >> <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com> > >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > >> > >> Laszlo, > >> > >> Thanks for the detailed response on the patch. Always happy to learn > >> about stuff from the past. > >> > >> Liming, > >> > >> I am currently the maintainer of NetworkPkg :) If you require additional > >> feedback from Siyuan or/and Jiaxin, that's ok. > >> Please let me know if any corrections to the patch (like CVE note) are > >> required from your point of view. > >> > >> Thanks, > >> Maciej > >> > >> On 28-Feb-20 03:59, Liming Gao wrote: > >>> Also include NetworkPkg reviewer to collect the feedback for this change. > >>> > >>> Thanks > >>> Liming > >>>> -----Original Message----- > >>>> From: Laszlo Ersek <lersek@redhat.com> > >>>> Sent: Friday, February 28, 2020 1:40 AM > >>>> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D > >>>> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org> > >>>> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com> > >>>> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. > >>>> > >>>> On 02/27/20 14:14, Laszlo Ersek wrote: > >>>>> (+Liming and stewards; CC Nick) > >>>>> > >>>>> On 02/27/20 12:02, Maciej Rabeda wrote: > >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > >>>>>> > >>>>>> 'ping' command's receive flow utilizes a single Rx token which it > >>>>>> attempts to reuse before recycling the previously received packet. > >>>>>> This causes a situation where under ICMP traffic, > >>>>>> Ping6OnEchoReplyReceived() function will receive an already > >>>>>> recycled packet with EFI_SUCCESS token status and finally > >>>>>> dereference invalid pointers from RxData structure. > >>>>>> > >>>>>> Cc: Ray Ni <ray.ni@intel.com> > >>>>>> Cc: Zhichao Gao <zhichao.gao@intel.com> > >>>>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > >>>>>> --- > >>>>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > >>>>>> 1 file changed, 5 insertions(+), 4 deletions(-) > >>>>>> > >>>>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>>>> index 23567fa2c1bb..a3fa32515192 100644 > >>>>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > >>>>>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > >>>>>> > >>>>>> ON_EXIT: > >>>>>> > >>>>>> + // > >>>>>> + // Recycle the packet before reusing RxToken > >>>>>> + // > >>>>>> + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >>>>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >>>>>> + > >>>>>> if (Private->RxCount < Private->SendNum) { > >>>>>> // > >>>>>> // Continue to receive icmp echo reply packets. > >>>>>> @@ -632,10 +637,6 @@ ON_EXIT: > >>>>>> // > >>>>>> Private->Status = EFI_SUCCESS; > >>>>>> } > >>>>>> - // > >>>>>> - // Singal to recycle the each rxdata here, not at the end of process. > >>>>>> - // > >>>>>> - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)- > >>>>> RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > >>>>>> } > >>>>>> > >>>>>> /** > >>>>>> > >>>>> (1) This patch proposes to fix one of the BZs (2032) that fall under > >>>>> CVE-2019-14559 (joint tracker: 2550). > >>>>> > >>>>> Consequently: > >>>>> > >>>>> (1a) Do we want to include this in the upcoming stable tag? > >>>>> > >>>>> If so, we might want to extend the hard feature freeze by a few days. > >>>>> > >>>>> (1b) Please append the string " (CVE-2019-14559)" -- note the separating > >>>>> space! -- to the subject line. > >>>>> > >>>>> (2) However: I remember from an earlier Bugzilla entry (can't tell > >>>>> off-hand, which one, sorry) that ShellPkg issues are *never* considered > >>>>> CVE-worthy, because the shell is not considered a "production element" > >>>>> of the UEFI boot path. > >>>> I misremembered -- there is indeed a comment like that, in the TianoCore > >>>> bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which > >>>> has since been split off to the edk2-libc project): > >>>> > >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1 > >>>> > >>>> StdLib is supposed to be used only by applications in shell, all of > >>>> which are meant for debug, diagnosis and/or test purpose, not for > >>>> product UEFI BIOS. Any issue in it will not be taken as security > >>>> issue but just normal bug. > >>>> > >>>> Sorry about causing confusion. So, the ShellPkg maintainers should > >>>> decide what to do about this bug (keep it under the CVE scope vs. > >>>> exclude it from the CVE scope; and then, propose it for the stable tag > >>>> or merge it afterwards). > >>>> > >>>> One data point: the bug appears to go back to the inception of the Ping > >>>> command, in historical commit 68fb05272b45 ("Add Network1 profile.", > >>>> 2011-03-25). It's not a new bug, it seems. > >>>> > >>>> Thanks > >>>> Laszlo > >>>> > >>>>> TianoCore#2032 was originally filed for NetworkPkg, and indeed that > >>>>> seemed to justify the CVE assignment. However, now that Nick's and > >>>>> Maciej's analysis shows that NetworkPkg is unaffected (and we know, per > >>>>> above, that ShellPkg is not CVE-worthy), should we rather *remove* this > >>>>> BZ from the CVE-2019-14559 umbrella? > >>>>> > >>>>> Because, in that case, modifying the subject line on the patch is not > >>>>> necessary; and more importantly, we might not even want to put this into > >>>>> edk2-stable202002. (It's still a bugfix, but may not be important enough.) > >>>>> > >>>>> Thanks! > >>>>> Laszlo > >>>>> > >>> > >>> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-02-27 11:02 [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow Maciej Rabeda 2020-02-27 13:14 ` [edk2-devel] " Laszlo Ersek @ 2020-03-25 11:34 ` Laszlo Ersek 2020-03-26 3:16 ` Gao, Zhichao 2020-03-31 11:53 ` Siyuan, Fu 1 sibling, 2 replies; 14+ messages in thread From: Laszlo Ersek @ 2020-03-25 11:34 UTC (permalink / raw) To: Ray Ni, Zhichao Gao; +Cc: devel, maciej.rabeda Ray, Zhichao, On 02/27/20 12:02, Maciej Rabeda wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > > 'ping' command's receive flow utilizes a single Rx token which it > attempts to reuse before recycling the previously received packet. > This causes a situation where under ICMP traffic, > Ping6OnEchoReplyReceived() function will receive an already > recycled packet with EFI_SUCCESS token status and finally > dereference invalid pointers from RxData structure. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > --- > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) can you please review this ShellPkg patch? It's been on the list for almost a month now. Thanks Laszlo > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > index 23567fa2c1bb..a3fa32515192 100644 > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > > ON_EXIT: > > + // > + // Recycle the packet before reusing RxToken > + // > + gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > + > if (Private->RxCount < Private->SendNum) { > // > // Continue to receive icmp echo reply packets. > @@ -632,10 +637,6 @@ ON_EXIT: > // > Private->Status = EFI_SUCCESS; > } > - // > - // Singal to recycle the each rxdata here, not at the end of process. > - // > - gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal); > } > > /** > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-03-25 11:34 ` Laszlo Ersek @ 2020-03-26 3:16 ` Gao, Zhichao 2020-03-31 9:03 ` Maciej Rabeda 2020-03-31 11:53 ` Siyuan, Fu 1 sibling, 1 reply; 14+ messages in thread From: Gao, Zhichao @ 2020-03-26 3:16 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ray, Wu, Jiaxin, Fu, Siyuan Cc: devel@edk2.groups.io, maciej.rabeda@linux.intel.com The ping command implementation would go into the ip4/ip6 protocol. But I am not familiar with the network part. Jiaxin/Siyuan, Can you help to review this patch? Thanks, Zhichao > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, March 25, 2020 7:34 PM > To: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 > receive flow. > > Ray, Zhichao, > > On 02/27/20 12:02, Maciej Rabeda wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > > > > 'ping' command's receive flow utilizes a single Rx token which it > > attempts to reuse before recycling the previously received packet. > > This causes a situation where under ICMP traffic, > > Ping6OnEchoReplyReceived() function will receive an already recycled > > packet with EFI_SUCCESS token status and finally dereference invalid > > pointers from RxData structure. > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > > --- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > can you please review this ShellPkg patch? It's been on the list for almost a > month now. > > Thanks > Laszlo > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > index 23567fa2c1bb..a3fa32515192 100644 > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > > > > ON_EXIT: > > > > + // > > + // Recycle the packet before reusing RxToken // gBS->SignalEvent > > + (Private->IpChoice == > > + PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > >RxToken.Packet.R > > + xData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- > >RxToken.Packe > > + t.RxData)->RecycleSignal); > > + > > if (Private->RxCount < Private->SendNum) { > > // > > // Continue to receive icmp echo reply packets. > > @@ -632,10 +637,6 @@ ON_EXIT: > > // > > Private->Status = EFI_SUCCESS; > > } > > - // > > - // Singal to recycle the each rxdata here, not at the end of process. > > - // > > - gBS->SignalEvent (Private->IpChoice == > > PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > >RxToken.Packet.RxD > > ata)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- > >RxToken.Packet.Rx > > Data)->RecycleSignal); > > } > > > > /** > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-03-26 3:16 ` Gao, Zhichao @ 2020-03-31 9:03 ` Maciej Rabeda 0 siblings, 0 replies; 14+ messages in thread From: Maciej Rabeda @ 2020-03-31 9:03 UTC (permalink / raw) To: devel, zhichao.gao, Laszlo Ersek, Ni, Ray, Wu, Jiaxin, Fu, Siyuan Siyuan/Jiaxin, Can we get this patch reviewed? I cannot give myself green light on this patch :) On 26-Mar-20 04:16, Gao, Zhichao wrote: > The ping command implementation would go into the ip4/ip6 protocol. But I am not familiar with the network part. > > Jiaxin/Siyuan, > > Can you help to review this patch? > > Thanks, > Zhichao > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Wednesday, March 25, 2020 7:34 PM >> To: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> >> Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 >> receive flow. >> >> Ray, Zhichao, >> >> On 02/27/20 12:02, Maciej Rabeda wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 >>> >>> 'ping' command's receive flow utilizes a single Rx token which it >>> attempts to reuse before recycling the previously received packet. >>> This causes a situation where under ICMP traffic, >>> Ping6OnEchoReplyReceived() function will receive an already recycled >>> packet with EFI_SUCCESS token status and finally dereference invalid >>> pointers from RxData structure. >>> >>> Cc: Ray Ni <ray.ni@intel.com> >>> Cc: Zhichao Gao <zhichao.gao@intel.com> >>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>> --- >>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >> can you please review this ShellPkg patch? It's been on the list for almost a >> month now. >> >> Thanks >> Laszlo >> >>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>> index 23567fa2c1bb..a3fa32515192 100644 >>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( >>> >>> ON_EXIT: >>> >>> + // >>> + // Recycle the packet before reusing RxToken // gBS->SignalEvent >>> + (Private->IpChoice == >>> + PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- >>> RxToken.Packet.R >>> + xData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- >>> RxToken.Packe >>> + t.RxData)->RecycleSignal); >>> + >>> if (Private->RxCount < Private->SendNum) { >>> // >>> // Continue to receive icmp echo reply packets. >>> @@ -632,10 +637,6 @@ ON_EXIT: >>> // >>> Private->Status = EFI_SUCCESS; >>> } >>> - // >>> - // Singal to recycle the each rxdata here, not at the end of process. >>> - // >>> - gBS->SignalEvent (Private->IpChoice == >>> PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- >>> RxToken.Packet.RxD >>> ata)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- >>> RxToken.Packet.Rx >>> Data)->RecycleSignal); >>> } >>> >>> /** >>> > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-03-25 11:34 ` Laszlo Ersek 2020-03-26 3:16 ` Gao, Zhichao @ 2020-03-31 11:53 ` Siyuan, Fu 2020-03-31 14:50 ` Gao, Zhichao 1 sibling, 1 reply; 14+ messages in thread From: Siyuan, Fu @ 2020-03-31 11:53 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, Gao, Zhichao Cc: maciej.rabeda@linux.intel.com 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年3月25日 19:34 > To: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive > flow. > > Ray, Zhichao, > > On 02/27/20 12:02, Maciej Rabeda wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > > > > 'ping' command's receive flow utilizes a single Rx token which it > > attempts to reuse before recycling the previously received packet. > > This causes a situation where under ICMP traffic, > > Ping6OnEchoReplyReceived() function will receive an already > > recycled packet with EFI_SUCCESS token status and finally > > dereference invalid pointers from RxData structure. > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > > --- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > can you please review this ShellPkg patch? It's been on the list for > almost a month now. > > Thanks > Laszlo > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > index 23567fa2c1bb..a3fa32515192 100644 > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > > > > ON_EXIT: > > > > + // > > + // Recycle the packet before reusing RxToken > > + // > > + gBS->SignalEvent (Private->IpChoice == > PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > >RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- > >RxToken.Packet.RxData)->RecycleSignal); > > + > > if (Private->RxCount < Private->SendNum) { > > // > > // Continue to receive icmp echo reply packets. > > @@ -632,10 +637,6 @@ ON_EXIT: > > // > > Private->Status = EFI_SUCCESS; > > } > > - // > > - // Singal to recycle the each rxdata here, not at the end of process. > > - // > > - gBS->SignalEvent (Private->IpChoice == > PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > >RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private- > >RxToken.Packet.RxData)->RecycleSignal); > > } > > > > /** > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-03-31 11:53 ` Siyuan, Fu @ 2020-03-31 14:50 ` Gao, Zhichao 2020-03-31 17:42 ` Maciej Rabeda 0 siblings, 1 reply; 14+ messages in thread From: Gao, Zhichao @ 2020-03-31 14:50 UTC (permalink / raw) To: Fu, Siyuan, devel@edk2.groups.io, lersek@redhat.com, Ni, Ray Cc: maciej.rabeda@linux.intel.com Acked-by: Zhichao Gao <zhichao.gao@intel.com> > -----Original Message----- > From: Fu, Siyuan <siyuan.fu@intel.com> > Sent: Tuesday, March 31, 2020 7:54 PM > To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; > Gao, Zhichao <zhichao.gao@intel.com> > Cc: maciej.rabeda@linux.intel.com > Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive > flow. > > 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年3月25日 19:34 > > To: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> > > Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com > > Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 > > receive flow. > > > > Ray, Zhichao, > > > > On 02/27/20 12:02, Maciej Rabeda wrote: > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 > > > > > > 'ping' command's receive flow utilizes a single Rx token which it > > > attempts to reuse before recycling the previously received packet. > > > This causes a situation where under ICMP traffic, > > > Ping6OnEchoReplyReceived() function will receive an already recycled > > > packet with EFI_SUCCESS token status and finally dereference invalid > > > pointers from RxData structure. > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Zhichao Gao <zhichao.gao@intel.com> > > > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > > > --- > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > can you please review this ShellPkg patch? It's been on the list for > > almost a month now. > > > > Thanks > > Laszlo > > > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > > index 23567fa2c1bb..a3fa32515192 100644 > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c > > > @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( > > > > > > ON_EXIT: > > > > > > + // > > > + // Recycle the packet before reusing RxToken // > > > + gBS->SignalEvent (Private->IpChoice == > > PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > > >RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private > > >- RxToken.Packet.RxData)->RecycleSignal); > > > + > > > if (Private->RxCount < Private->SendNum) { > > > // > > > // Continue to receive icmp echo reply packets. > > > @@ -632,10 +637,6 @@ ON_EXIT: > > > // > > > Private->Status = EFI_SUCCESS; > > > } > > > - // > > > - // Singal to recycle the each rxdata here, not at the end of process. > > > - // > > > - gBS->SignalEvent (Private->IpChoice == > > PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- > > >RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private > > >- RxToken.Packet.RxData)->RecycleSignal); > > > } > > > > > > /** > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. 2020-03-31 14:50 ` Gao, Zhichao @ 2020-03-31 17:42 ` Maciej Rabeda 0 siblings, 0 replies; 14+ messages in thread From: Maciej Rabeda @ 2020-03-31 17:42 UTC (permalink / raw) To: devel, zhichao.gao, Fu, Siyuan, lersek@redhat.com, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 3155 bytes --] Much appreciated, I'll submit the patch tomorrow :) On 31-Mar-20 16:50, Gao, Zhichao wrote: > Acked-by: Zhichao Gao <zhichao.gao@intel.com> > >> -----Original Message----- >> From: Fu, Siyuan <siyuan.fu@intel.com> >> Sent: Tuesday, March 31, 2020 7:54 PM >> To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; >> Gao, Zhichao <zhichao.gao@intel.com> >> Cc: maciej.rabeda@linux.intel.com >> Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive >> flow. >> >> 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年3月25日 19:34 >>> To: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com> >>> Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com >>> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 >>> receive flow. >>> >>> Ray, Zhichao, >>> >>> On 02/27/20 12:02, Maciej Rabeda wrote: >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032 >>>> >>>> 'ping' command's receive flow utilizes a single Rx token which it >>>> attempts to reuse before recycling the previously received packet. >>>> This causes a situation where under ICMP traffic, >>>> Ping6OnEchoReplyReceived() function will receive an already recycled >>>> packet with EFI_SUCCESS token status and finally dereference invalid >>>> pointers from RxData structure. >>>> >>>> Cc: Ray Ni <ray.ni@intel.com> >>>> Cc: Zhichao Gao <zhichao.gao@intel.com> >>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>>> --- >>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++---- >>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> can you please review this ShellPkg patch? It's been on the list for >>> almost a month now. >>> >>> Thanks >>> Laszlo >>> >>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> index 23567fa2c1bb..a3fa32515192 100644 >>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c >>>> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived ( >>>> >>>> ON_EXIT: >>>> >>>> + // >>>> + // Recycle the packet before reusing RxToken // >>>> + gBS->SignalEvent (Private->IpChoice == >>> PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- >>>> RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private >>>> - RxToken.Packet.RxData)->RecycleSignal); >>>> + >>>> if (Private->RxCount < Private->SendNum) { >>>> // >>>> // Continue to receive icmp echo reply packets. >>>> @@ -632,10 +637,6 @@ ON_EXIT: >>>> // >>>> Private->Status = EFI_SUCCESS; >>>> } >>>> - // >>>> - // Singal to recycle the each rxdata here, not at the end of process. >>>> - // >>>> - gBS->SignalEvent (Private->IpChoice == >>> PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private- >>>> RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private >>>> - RxToken.Packet.RxData)->RecycleSignal); >>>> } >>>> >>>> /** >>>> >>> >>> > > > [-- Attachment #2: Type: text/html, Size: 5729 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-03-31 17:42 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-27 11:02 [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow Maciej Rabeda 2020-02-27 13:14 ` [edk2-devel] " Laszlo Ersek 2020-02-27 17:40 ` Laszlo Ersek 2020-02-28 2:59 ` Liming Gao 2020-02-28 11:41 ` Maciej Rabeda 2020-02-28 11:50 ` Liming Gao 2020-02-28 12:35 ` Maciej Rabeda 2020-03-02 13:43 ` Liming Gao 2020-03-25 11:34 ` Laszlo Ersek 2020-03-26 3:16 ` Gao, Zhichao 2020-03-31 9:03 ` Maciej Rabeda 2020-03-31 11:53 ` Siyuan, Fu 2020-03-31 14:50 ` Gao, Zhichao 2020-03-31 17:42 ` Maciej Rabeda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox