Another minor comment. Ip4Dxe is moved into NetworkPkg. So, the patch title should be NetworkPkg/Ip4Dxe.
Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Monday, February 17, 2020 6:40 PM > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin...@intel.com> > Cc: Fu, Siyuan <siyuan...@intel.com>; Maciej Rabeda > <maciej.rab...@linux.intel.com>; Armour, Nicholas <nicholas.arm...@intel.com> > Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received > package length (CVE-2019-14559). > > On 02/17/20 08:43, Wu, Jiaxin wrote: > > This patch is to check the received package length to make sure the package > > has a valid length field. > > > > Cc: Fu Siyuan <siyuan...@intel.com> > > Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> > > Signed-off-by: Wu Jiaxin <jiaxin...@intel.com> > > Reviewed-by: Siyuan Fu <siyuan...@intel.com> > > --- > > NetworkPkg/Ip4Dxe/Ip4Input.c | 46 > > +++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 37 insertions(+), 9 deletions(-) > > There are two patches on the list for CVE-2019-14559: > > - [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package > length (CVE-2019-14559). > - [edk2-devel] [PATCH 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP > packets(CVE-2019-14559). > > sent by different submitters. > > How do they relate to each other? > > Also, while Nick's patch mentions TianoCore#2031, the current patch doesn't > include a BZ link. Is the current patch for TianoCore#2032? > (Per <https://bugzilla.tianocore.org/show_bug.cgi?id=2032#c8>, both BZs share > the same CVE ID.) > > Also, I remain confused (with comment 11 being the latest one, as of this > time, in TianoCore#2032), whether the issue affects IPv4 only, > IPv6 only, or both. This patch is only for IPv4, apparently. > > If the present patch is related to TianoCore#2032, then please add a mailing > list archive link to the BZ, and move the BZ to IN_PROGRESS > status. > > Laszlo > > > > > diff --git a/NetworkPkg/Ip4Dxe/Ip4Input.c b/NetworkPkg/Ip4Dxe/Ip4Input.c > > index fec242c71f..95fbd01d05 100644 > > --- a/NetworkPkg/Ip4Dxe/Ip4Input.c > > +++ b/NetworkPkg/Ip4Dxe/Ip4Input.c > > @@ -1,9 +1,9 @@ > > /** @file > > IP4 input process. > > > > -Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2005 - 2020, Intel Corporation. All rights reserved.<BR> > > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR> > > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -709,14 +709,10 @@ Ip4PreProcessPacket ( > > UINT16 Checksum; > > > > // > > // Check if the IP4 header is correctly formatted. > > // > > - if ((*Packet)->TotalSize < IP4_MIN_HEADLEN) { > > - return EFI_INVALID_PARAMETER; > > - } > > - > > HeadLen = (Head->HeadLen << 2); > > TotalLen = NTOHS (Head->TotalLen); > > > > // > > // Mnp may deliver frame trailer sequence up, trim it off. > > @@ -806,10 +802,34 @@ Ip4PreProcessPacket ( > > } > > > > return EFI_SUCCESS; > > } > > > > +/** > > + This function checks the IPv4 packet length. > > + > > + @param[in] Packet Pointer to the IPv4 Packet to be > > checked. > > + > > + @retval TRUE The input IPv4 packet length is valid. > > + @retval FALSE The input IPv4 packet length is invalid. > > + > > +**/ > > +BOOLEAN > > +Ip4IsValidPacketLength ( > > + IN NET_BUF *Packet > > + ) > > +{ > > + // > > + // Check the IP4 packet length. > > + // > > + if (Packet->TotalSize < IP4_MIN_HEADLEN) { > > + return FALSE; > > + } > > + > > + return TRUE; > > +} > > + > > /** > > The IP4 input routine. It is called by the IP4_INTERFACE when a > > IP4 fragment is received from MNP. > > > > @param[in] Ip4Instance The IP4 child that request the receive, > > most like > > @@ -842,10 +862,14 @@ Ip4AccpetFrame ( > > > > if (EFI_ERROR (IoStatus) || (IpSb->State == IP4_SERVICE_DESTROY)) { > > goto DROP; > > } > > > > + if (!Ip4IsValidPacketLength (Packet)) { > > + goto RESTART; > > + } > > + > > Head = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL); > > ASSERT (Head != NULL); > > OptionLen = (Head->HeadLen << 2) - IP4_MIN_HEADLEN; > > if (OptionLen > 0) { > > Option = (UINT8 *) (Head + 1); > > @@ -888,14 +912,18 @@ Ip4AccpetFrame ( > > // > > // If the packet is protected by tunnel mode, parse the inner Ip Packet. > > // > > ZeroMem (&ZeroHead, sizeof (IP4_HEAD)); > > if (0 == CompareMem (Head, &ZeroHead, sizeof (IP4_HEAD))) { > > - // Packet may have been changed. Head, HeadLen, TotalLen, and > > - // info must be reloaded before use. The ownership of the packet > > - // is transferred to the packet process logic. > > - // > > + // Packet may have been changed. Head, HeadLen, TotalLen, and > > + // info must be reloaded before use. The ownership of the packet > > + // is transferred to the packet process logic. > > + // > > + if (!Ip4IsValidPacketLength (Packet)) { > > + goto RESTART; > > + } > > + > > Head = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL); > > ASSERT (Head != NULL); > > Status = Ip4PreProcessPacket ( > > IpSb, > > &Packet, > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54554): https://edk2.groups.io/g/devel/message/54554 Mute This Topic: https://groups.io/mt/71345472/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
