Re: [edk2] [PATCH v2 2/3] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.
On 11/06/18 01:54, Fu, Siyuan wrote:
> Hi, Laszlo
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> I'm not asking for additional documentation regarding this fact, given
>> commit 0bcbdf9c7445 ("NetworkPkg/IScsiDxe: Add the clarification
>> compared to IScsiDxe in MdeModulePkg.", 2018-09-27). I'm just asking if
>> we've considered this and find it acceptable.
>
> The MdeModulePkg iSCSI driver can be built without OpenSSL because it writes
> its own crypto functions in IScsiDxe/Md5.c, that's not allowed by
> current edk2 security development guide line. We should always use the
> crypto APIs provided by OpenSSL instead of reinvent a new implementation.
Ah, that makes a lot of sense. I've wondered about this difference in
dependencies for a long time.
[...]
Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/3] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.
Hi, Laszlo
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, November 6, 2018 6:37 AM
> To: Fu, Siyuan
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v2 2/3] ArmVirtPkg: Replace obsoleted network
> drivers from platform DSC/FDF.
>
> On 11/05/18 11:49, Fu Siyuan wrote:
> > V2:
> > Add missing library instance for NetworkPkg iSCSI driver.
> >
> > This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with
> those
> > ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being
> actively
> > maintained and will be removed from edk2 master soon.
> >
> > Cc: Laszlo Ersek
> > Cc: Ard Biesheuvel
> > Cc: Julien Grall
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Fu Siyuan
> > ---
> > ArmVirtPkg/ArmVirtQemu.dsc | 13 ++---
> > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++---
> > ArmVirtPkg/ArmVirtQemuKernel.dsc | 13 ++---
> > 3 files changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 885c6b14b844..0f403973bea0 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -70,6 +70,9 @@ [LibraryClasses.common.PEIM]
> >
> > [LibraryClasses.common.UEFI_DRIVER]
> >UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>
> (1) I couldn't participate in the discussion last week; I was away. On
> my return, I have now seen multiple related threads. I guess I can
> describe my general concern here.
>
> My general concern is that the edk2 network stack can no longer be built
> without OpenSSL. (As long as we include the iSCSI driver in the "edk2
> network stack".) Is that intentional?
>
> I'm not asking for additional documentation regarding this fact, given
> commit 0bcbdf9c7445 ("NetworkPkg/IScsiDxe: Add the clarification
> compared to IScsiDxe in MdeModulePkg.", 2018-09-27). I'm just asking if
> we've considered this and find it acceptable.
The MdeModulePkg iSCSI driver can be built without OpenSSL because it writes
its own crypto functions in IScsiDxe/Md5.c, that's not allowed by
current edk2 security development guide line. We should always use the
crypto APIs provided by OpenSSL instead of reinvent a new implementation.
So yes, it's the intentional, and even if we decide not to delete the
MdeModulePkg iSCSI driver, there will be another patch to delete its own
Md5.c and use OpenSSL API.
>
>
> (2) Once we remove the IPv4-only drivers, the INF file comments added in:
>
> 897720daef33 NetworkPkg/TcpDxe: Add the clarification compared to
>Tcp4Dxe in MdeModulePkg.
> 0bcbdf9c7445 NetworkPkg/IScsiDxe: Add the clarification compared to
>IScsiDxe in MdeModulePkg.
> 24c55f5dcc31 NetworkPkg/UefiPxeBcDxe: Add the clarification compared
>to UefiPxeBcDxe in MdeModulePkg.
>
> should be updated, because the comparisons to MdeModulePkg drivers will
> no longer make sense.
You are right, my V1 patch 6/7 deletes the MdeModulePkg code and 7/7 removes
these comments in NetworkPkg drivers' INF file. Since we decided to keep
the MdeModulePkg driver in edk2-stable201811 tag, I removed the 6/7 and 7/7
in v2 patch. These changes will be sent out in a separate patch later.
I will try to include more details in the patch description in future
to let you known the background.
>
>
> (3) These library class resolutions are already spelled out in
> "ArmVirtPkg/ArmVirt.dsc.inc". Please see under the comment
>
> #
> # CryptoPkg libraries needed by multiple firmware features
> #
>
> We shouldn't duplicate those lib class resolutions.
Just see it, will send out a v3 patch for the ARM package.
>
>
> (4) In particular, the "CryptoPkg/Library/OpensslLib/OpensslLib.inf"
> instance contains TLS support, and it is overkill for just IPv6. The
> "OpensslLibCrypto.inf" instance is sufficient.
>
TLS is required by HTTP boot driver so I put OpensslLib instance as
default, but it's a good suggestion. I will update the NetworkPkg Wiki
page to tell platform owners to use the smaller crypto library instance
if they don't need HTTP.
> >
> >
> ##
> ##
> > #
> > @@ -346,18 +349,14 @@ [Components.common]
> >
Re: [edk2] [PATCH v2 2/3] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.
On 11/05/18 11:49, Fu Siyuan wrote:
> V2:
> Add missing library instance for NetworkPkg iSCSI driver.
>
> This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with those
> ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being actively
> maintained and will be removed from edk2 master soon.
>
> Cc: Laszlo Ersek
> Cc: Ard Biesheuvel
> Cc: Julien Grall
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Fu Siyuan
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 13 ++---
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++---
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 13 ++---
> 3 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 885c6b14b844..0f403973bea0 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -70,6 +70,9 @@ [LibraryClasses.common.PEIM]
>
> [LibraryClasses.common.UEFI_DRIVER]
>UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
(1) I couldn't participate in the discussion last week; I was away. On
my return, I have now seen multiple related threads. I guess I can
describe my general concern here.
My general concern is that the edk2 network stack can no longer be built
without OpenSSL. (As long as we include the iSCSI driver in the "edk2
network stack".) Is that intentional?
I'm not asking for additional documentation regarding this fact, given
commit 0bcbdf9c7445 ("NetworkPkg/IScsiDxe: Add the clarification
compared to IScsiDxe in MdeModulePkg.", 2018-09-27). I'm just asking if
we've considered this and find it acceptable.
(2) Once we remove the IPv4-only drivers, the INF file comments added in:
897720daef33 NetworkPkg/TcpDxe: Add the clarification compared to
Tcp4Dxe in MdeModulePkg.
0bcbdf9c7445 NetworkPkg/IScsiDxe: Add the clarification compared to
IScsiDxe in MdeModulePkg.
24c55f5dcc31 NetworkPkg/UefiPxeBcDxe: Add the clarification compared
to UefiPxeBcDxe in MdeModulePkg.
should be updated, because the comparisons to MdeModulePkg drivers will
no longer make sense.
(3) These library class resolutions are already spelled out in
"ArmVirtPkg/ArmVirt.dsc.inc". Please see under the comment
#
# CryptoPkg libraries needed by multiple firmware features
#
We shouldn't duplicate those lib class resolutions.
(4) In particular, the "CryptoPkg/Library/OpensslLib/OpensslLib.inf"
instance contains TLS support, and it is overkill for just IPv6. The
"OpensslLibCrypto.inf" instance is sufficient.
>
>
>
> #
> @@ -346,18 +349,14 @@ [Components.common]
>MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
> + NetworkPkg/TcpDxe/TcpDxe.inf
> + NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> + NetworkPkg/IScsiDxe/IScsiDxe.inf
> !if $(NETWORK_IP6_ENABLE) == TRUE
>NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> - NetworkPkg/TcpDxe/TcpDxe.inf
>NetworkPkg/Udp6Dxe/Udp6Dxe.inf
>NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
>NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> - NetworkPkg/IScsiDxe/IScsiDxe.inf
> -!else
> - MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> - MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> !endif
> !if $(HTTP_BOOT_ENABLE) == TRUE
>NetworkPkg/DnsDxe/DnsDxe.inf
(5) This change will break the build (without NETWORK_IP6_ENABLE).
Namely, "NetworkPkg/IScsiDxe/IScsiDxe.inf" would be compiled
unconditionally. However, "NetworkPkg/IScsiDxe/IScsiDxe.inf" depends on
the TcpIoLib class -- as the sole driver in the edk2 tree --, and we
only resolve that lib class (in "ArmVirtPkg/ArmVirt.dsc.inc") if
NETWORK_IP6_ENABLE is defined.
* If we decide that "NetworkPkg/IScsiDxe/IScsiDxe.inf" is an integral
part of the edk2 network driver stack (even without NETWORK_IP6_ENABLE),
then:
- we should make the current TcpIoLib class resolution unconditional,
- we should make the current IntrinsicLib / OpensslLib / BaseCryptLib
resolutions unconditional,
* Otherwise (= if we consider "NetworkPkg/IScsiDxe/IScsiDxe.inf"
optional for networking), we should introduce NETWORK_ISCSI_ENABLE, and
*replace* NETWORK_IP6_ENABLE with NETWORK_ISCSI_ENABLE in the above lib
class resolutions. (And also make the DSC / FDF inclusion of
"NetworkPkg/IScsiDxe/IScsiDxe.inf" dependent on the new
NETWORK_ISCSI_ENABLE.)
Thanks
Laszlo
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index a6390bd4b841..3316f982695f 100644
> ---
[edk2] [PATCH v2 2/3] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.
V2: Add missing library instance for NetworkPkg iSCSI driver. This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with those ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being actively maintained and will be removed from edk2 master soon. Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Julien Grall Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Fu Siyuan --- ArmVirtPkg/ArmVirtQemu.dsc | 13 ++--- ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++--- ArmVirtPkg/ArmVirtQemuKernel.dsc | 13 ++--- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc index 885c6b14b844..0f403973bea0 100644 --- a/ArmVirtPkg/ArmVirtQemu.dsc +++ b/ArmVirtPkg/ArmVirtQemu.dsc @@ -70,6 +70,9 @@ [LibraryClasses.common.PEIM] [LibraryClasses.common.UEFI_DRIVER] UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf # @@ -346,18 +349,14 @@ [Components.common] MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf + NetworkPkg/TcpDxe/TcpDxe.inf + NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf + NetworkPkg/IScsiDxe/IScsiDxe.inf !if $(NETWORK_IP6_ENABLE) == TRUE NetworkPkg/Ip6Dxe/Ip6Dxe.inf - NetworkPkg/TcpDxe/TcpDxe.inf NetworkPkg/Udp6Dxe/Udp6Dxe.inf NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf - NetworkPkg/IScsiDxe/IScsiDxe.inf -!else - MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf - MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf !endif !if $(HTTP_BOOT_ENABLE) == TRUE NetworkPkg/DnsDxe/DnsDxe.inf diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc index a6390bd4b841..3316f982695f 100644 --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc @@ -126,18 +126,14 @@ [FV.FvMain] INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf + INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf + INF NetworkPkg/IScsiDxe/IScsiDxe.inf + INF NetworkPkg/TcpDxe/TcpDxe.inf !if $(NETWORK_IP6_ENABLE) == TRUE INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf - INF NetworkPkg/TcpDxe/TcpDxe.inf INF NetworkPkg/Udp6Dxe/Udp6Dxe.inf INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf - INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf - INF NetworkPkg/IScsiDxe/IScsiDxe.inf -!else - INF MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf - INF MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf - INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf !endif !if $(HTTP_BOOT_ENABLE) == TRUE INF NetworkPkg/DnsDxe/DnsDxe.inf diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc index 434d6861a56f..4920a66f2fdb 100644 --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc @@ -67,6 +67,9 @@ [LibraryClasses.common] [LibraryClasses.common.UEFI_DRIVER] UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf + IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf [BuildOptions.ARM.EDKII.SEC, BuildOptions.ARM.EDKII.BASE] # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE @@ -335,18 +338,14 @@ [Components.common] MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf + NetworkPkg/TcpDxe/TcpDxe.inf + NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf + NetworkPkg/IScsiDxe/IScsiDxe.inf !if $(NETWORK_IP6_ENABLE) == TRUE NetworkPkg/Ip6Dxe/Ip6Dxe.inf - NetworkPkg/TcpDxe/TcpDxe.inf NetworkPkg/Udp6Dxe/Udp6Dxe.inf NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf - NetworkPkg/IScsiDxe/IScsiDxe.inf -!else - MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf - MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf - MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf !endif !if $(HTTP_BOOT_ENABLE) == TRUE NetworkPkg/DnsDxe/DnsDxe.inf -- 2.19.1.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
