From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web09.13091.1582890108144558794 for ; Fri, 28 Feb 2020 03:41:48 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.126, mailfrom: maciej.rabeda@linux.intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Feb 2020 03:41:47 -0800 X-IronPort-AV: E=Sophos;i="5.70,495,1574150400"; d="scan'208";a="242357373" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.102.8.43]) ([10.102.8.43]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 28 Feb 2020 03:41:44 -0800 Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow. To: devel@edk2.groups.io, liming.gao@intel.com, Laszlo Ersek , "Kinney, Michael D" , Andrew Fish , "Leif Lindholm (Linaro address)" Cc: "Ni, Ray" , "Gao, Zhichao" , "Armour, Nicholas" , "Fu, Siyuan" , "Wu, Jiaxin" References: <20200227110212.1070-1-maciej.rabeda@linux.intel.com> <4dcf2f0b-c86e-7533-3428-ad07e9129f2d@redhat.com> <3e3521ab1a17472a921f238db1f8dd9b@intel.com> From: "Maciej Rabeda" Message-ID: <03ff9f49-f383-1fcf-bf1e-37a651521933@linux.intel.com> Date: Fri, 28 Feb 2020 12:41:39 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <3e3521ab1a17472a921f238db1f8dd9b@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl 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 >> Sent: Friday, February 28, 2020 1:40 AM >> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming ; Kinney, Michael D >> ; Andrew Fish ; Leif Lindholm (Linaro address) >> Cc: Ni, Ray ; Gao, Zhichao ; Armour, Nicholas >> 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 >>>> Cc: Zhichao Gao >>>> Signed-off-by: Maciej Rabeda >>>> --- >>>> 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 >>> > > >