From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.8915.1585752122090926389 for ; Wed, 01 Apr 2020 07:42:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SCOxX9b+; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585752121; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Yfkc74PWpNVy2tSprUhKR1lkfqYkVhgVw9FexciQQio=; b=SCOxX9b+Q+5LThRFVwqOf9+6Fj4Y9z0cNHtCcfzuhwRV3prqZlcN4NMl4K11EYU7p8xdyp Wq0dH5KgNghokGzHPXH1Jfckx1QEHbE1zOPNvCsurUzEzneYegibZu0jKZXHNTDnmn8zu1 Vr8LmYK66PTbuzoC9TUAro8ETZkkquk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-190-bCVV4jaJMryq6lNoJJ4EGQ-1; Wed, 01 Apr 2020 10:41:52 -0400 X-MC-Unique: bCVV4jaJMryq6lNoJJ4EGQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 66403100551A; Wed, 1 Apr 2020 14:41:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-94.ams2.redhat.com [10.36.114.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0E7585C1C5; Wed, 1 Apr 2020 14:41:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] NetworkPkg/UefiPxeBcDxe: handle competing DHCP servers (more) gracefully To: devel@edk2.groups.io, maciej.rabeda@linux.intel.com Cc: Jiaxin Wu , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Siyuan Fu References: <20200331004749.16128-1-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: <0bcfe946-eb0d-b647-aa7b-44d8e3b4f225@redhat.com> Date: Wed, 1 Apr 2020 16:41:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/31/20 19:49, Maciej Rabeda wrote: > Always better than not detecting such stuff at all (or by ASSERT in > debug builds). Thanks for the patch! >=20 > Reviewed-by: Maciej Rabeda Thanks All for the reviews; pushed as commit 3f55418d5396629c4458061f283068b6c46895fc, via . Laszlo >=20 > On 31-Mar-20 02:47, Laszlo Ersek wrote: >> When DHCP is misconfigured on a network segment, such that two DHCP >> servers attempt to reply to requests (and therefore race with each >> other), >> the edk2 PXE client can confuse itself. >> >> In PxeBcDhcp4BootInfo() / PxeBcDhcp6BootInfo(), the client may refer to= a >> DHCP reply packet as an "earlier" packet from the "same" DHCP server, >> when >> in reality both packets are unrelated, and arrive from different DHCP >> servers. >> >> While the edk2 PXE client can do nothing to fix this, it should at leas= t >> not ASSERT() -- ASSERT() is for catching programming errors >> (violations of >> invariants that are under the control of the programmer). ASSERT()s >> should >> in particular not refer to external data (such as network packets). >> What's >> more, in RELEASE builds, we get NULL pointer references. >> >> Check the problem conditions with actual "if"s, and return >> EFI_PROTOCOL_ERROR. This will trickle out to PxeBcLoadBootFile(), and b= e >> reported as "PXE-E99: Unexpected network error". >> >> Cc: Jiaxin Wu >> Cc: Maciej Rabeda >> Cc: Philippe Mathieu-Daud=C3=A9 >> Cc: Siyuan Fu >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> =C2=A0=C2=A0=C2=A0=C2=A0 Repo:=C2=A0=C2=A0 https://pagure.io/lersek/edk= 2.git >> =C2=A0=C2=A0=C2=A0=C2=A0 Branch: dhcp_assert >> >> =C2=A0 NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 30 ++++++++++++++++++-- >> =C2=A0 1 file changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >> b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >> index 10bbb06f7593..d062a526077b 100644 >> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >> @@ -482,7 +482,20 @@ PxeBcDhcp4BootInfo ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Cache4 =3D &Private->DhcpAck.Dhcp4; >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0 -=C2=A0 ASSERT (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] = !=3D NULL); >> +=C2=A0 if (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] =3D=3D NULL= ) { >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 // This should never happen in a correctly configur= ed DHCP / PXE >> +=C2=A0=C2=A0=C2=A0 // environment. One misconfiguration that can cause= it is two >> DHCP servers >> +=C2=A0=C2=A0=C2=A0 // mistakenly running on the same network segment a= t the same >> time, and >> +=C2=A0=C2=A0=C2=A0 // racing each other in answering DHCP requests. Th= us, the DHCP >> packets >> +=C2=A0=C2=A0=C2=A0 // that the edk2 PXE client considers "belonging to= gether" may >> actually be >> +=C2=A0=C2=A0=C2=A0 // entirely independent, coming from two (competing= ) DHCP servers. >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 // Try to deal with this gracefully. Note that this= check is not >> +=C2=A0=C2=A0=C2=A0 // comprehensive, as we don't try to identify all s= uch errors. >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 return EFI_PROTOCOL_ERROR; >> +=C2=A0 } >> =C2=A0 =C2=A0=C2=A0=C2=A0 // >> =C2=A0=C2=A0=C2=A0 // Parse the boot server address. >> @@ -612,7 +625,20 @@ PxeBcDhcp6BootInfo ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Cache6 =3D &Private->DhcpAck.Dhcp6; >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0 -=C2=A0 ASSERT (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] != = =3D NULL); >> +=C2=A0 if (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] =3D=3D NULL)= { >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 // This should never happen in a correctly configur= ed DHCP / PXE >> +=C2=A0=C2=A0=C2=A0 // environment. One misconfiguration that can cause= it is two >> DHCP servers >> +=C2=A0=C2=A0=C2=A0 // mistakenly running on the same network segment a= t the same >> time, and >> +=C2=A0=C2=A0=C2=A0 // racing each other in answering DHCP requests. Th= us, the DHCP >> packets >> +=C2=A0=C2=A0=C2=A0 // that the edk2 PXE client considers "belonging to= gether" may >> actually be >> +=C2=A0=C2=A0=C2=A0 // entirely independent, coming from two (competing= ) DHCP servers. >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 // Try to deal with this gracefully. Note that this= check is not >> +=C2=A0=C2=A0=C2=A0 // comprehensive, as we don't try to identify all s= uch errors. >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 return EFI_PROTOCOL_ERROR; >> +=C2=A0 } >> =C2=A0 =C2=A0=C2=A0=C2=A0 // >> =C2=A0=C2=A0=C2=A0 // Set the station address to IP layer. >=20 >=20 >=20