From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web12.1423.1585676981003198948 for ; Tue, 31 Mar 2020 10:49:41 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.151, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: 4eaQq/3hib7OpPF87EYsYO2k3q4uHavT6ZLcRE3jlTr4i2kbGi0VCOnTjvEpHkkX6NSZAcjQre POde5r9nCzDw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 10:49:40 -0700 IronPort-SDR: xNNnIzpuNUbWb5OwOSfZZQLRasQLvLc5ZYzGhRrLhv1+ENocxS9k1yHoxHYNPnhgGeNQbSoFWi Bd7WtPyBk5eQ== X-IronPort-AV: E=Sophos;i="5.72,328,1580803200"; d="scan'208";a="272845608" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.8.245]) ([10.213.8.245]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 10:49:39 -0700 Subject: Re: [edk2-devel] [PATCH] NetworkPkg/UefiPxeBcDxe: handle competing DHCP servers (more) gracefully To: devel@edk2.groups.io, lersek@redhat.com Cc: Jiaxin Wu , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Siyuan Fu References: <20200331004749.16128-1-lersek@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Tue, 31 Mar 2020 19:49:17 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200331004749.16128-1-lersek@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl Always better than not detecting such stuff at all (or by ASSERT in debug builds). Thanks for the patch! Reviewed-by: Maciej Rabeda 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 least > 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 be > reported as "PXE-E99: Unexpected network error". > > Cc: Jiaxin Wu > Cc: Maciej Rabeda > Cc: Philippe Mathieu-Daudé > Cc: Siyuan Fu > Signed-off-by: Laszlo Ersek > --- > > Notes: > Repo: https://pagure.io/lersek/edk2.git > Branch: dhcp_assert > > NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 30 ++++++++++++++++++-- > 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 ( > Cache4 = &Private->DhcpAck.Dhcp4; > } > > - ASSERT (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] != NULL); > + if (Cache4->OptList[PXEBC_DHCP4_TAG_INDEX_BOOTFILE] == NULL) { > + // > + // This should never happen in a correctly configured DHCP / PXE > + // environment. One misconfiguration that can cause it is two DHCP servers > + // mistakenly running on the same network segment at the same time, and > + // racing each other in answering DHCP requests. Thus, the DHCP packets > + // that the edk2 PXE client considers "belonging together" may actually be > + // entirely independent, coming from two (competing) DHCP servers. > + // > + // Try to deal with this gracefully. Note that this check is not > + // comprehensive, as we don't try to identify all such errors. > + // > + return EFI_PROTOCOL_ERROR; > + } > > // > // Parse the boot server address. > @@ -612,7 +625,20 @@ PxeBcDhcp6BootInfo ( > Cache6 = &Private->DhcpAck.Dhcp6; > } > > - ASSERT (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] != NULL); > + if (Cache6->OptList[PXEBC_DHCP6_IDX_BOOT_FILE_URL] == NULL) { > + // > + // This should never happen in a correctly configured DHCP / PXE > + // environment. One misconfiguration that can cause it is two DHCP servers > + // mistakenly running on the same network segment at the same time, and > + // racing each other in answering DHCP requests. Thus, the DHCP packets > + // that the edk2 PXE client considers "belonging together" may actually be > + // entirely independent, coming from two (competing) DHCP servers. > + // > + // Try to deal with this gracefully. Note that this check is not > + // comprehensive, as we don't try to identify all such errors. > + // > + return EFI_PROTOCOL_ERROR; > + } > > // > // Set the station address to IP layer.