Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

2024-05-29 Thread Gerd Hoffmann
On Thu, May 23, 2024 at 10:44:52PM GMT, Doug Flick via groups.io wrote:
> 
> REF:https://blog.quarkslab.com/pixiefail-nine-vulnerabilities-in-tianocores-edk-ii-ipv6-network-stack.html
> 
> This patch series patches the following CVEs:
> - CVE-2023-45236: Predictable TCP Initial Sequence Numbers
> - CVE-2023-45237: Use of a Weak PseudoRandom Number Generator

Ok, looks like there is some more fallout from this patch series which I
havn't seen in my initial testing.  It does not always happen, didn't
figure yet what exactly triggers the behavior.  But in some cases there
is quite some network stack activity, apparently done by
EVT_SIGNAL_EXIT_BOOT_SERVICES event handlers ...

With the debug patch below applied the tail of the ovmf log looks like
this:

  VirtioRngExitBoot: Context=0x7D73D798
  Hash2ServiceBindingDestroyChild - Invalid handle
  MnpServiceBindingDestroyChild: Failed to uninstall the ManagedNetwork 
protocol, Invalid Parameter.
  Support(): UNDI3.1 found on handle 7D461118
  Support(): supported on 7D461118
  Start(): UNDI3.1 found

  snp->undi.start()  1h:8000h
  InstallProtocolInterface: 7AB33A91-ACE5-4326-B572-E7EE33D39F16 7CE872C0
  InstallProtocolInterface: F44C00EE-1F2C-4A00-AA09-1C9F3E0800A3 7CE7D020
  Failed to generate random data using secure algorithm 0: Unsupported
  Failed to generate random data using secure algorithm 1: Unsupported
  Failed to generate random data using secure algorithm 2: Unsupported
  Failed to generate random data using secure algorithm 3: Unsupported
  VirtioRngGetRNG: not ready
  Failed to generate random data using secure algorithm 4: Device Error

  ASSERT_EFI_ERROR (Status = Device Error)
  ASSERT 
/home/kraxel/projects/edk2/NetworkPkg/Library/DxeNetLib/DxeNetLib.c(965): 
!(((INTN)(RETURN_STATUS)(Status)) < 0)

The VirtioRngDxe EVT_SIGNAL_EXIT_BOOT_SERVICES handler resets the
device, to make sure it will stop any DMA.

Once the reset is done the device can't deliver random numbers any more,
but the network code wants some.  So with the debug patch an assert is
triggered, without the debug patch the system simply hangs because the
virtio-rng device wouldn't answer request sent by the driver.

I'm wondering what the network code is actually doing here in the first
place?  It apparently /installs/ protocols in the
EVT_SIGNAL_EXIT_BOOT_SERVICES handler?  I don't think this is how things
are supposed to work ...

take care,
  Gerd

- cut here -
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
index 2da99540a208..3519521d6ab5 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
@@ -33,6 +33,7 @@ typedef struct {
   VRING Ring;   // VirtioRingInit   2
   EFI_RNG_PROTOCOL  Rng;// VirtioRngInit1
   VOID  *RingMap;   // VirtioRingMap2
+  BOOLEAN   Ready;
 } VIRTIO_RNG_DEV;
 
 #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 75a9644f749c..36e3eed4617c 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -77,7 +77,7 @@ VirtioNetExitBoot (
   //
   VNET_DEV  *Dev;
 
-  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __func__, Context));
+  DEBUG ((DEBUG_INFO, "%a: Context=0x%p\n", __func__, Context));
   Dev = Context;
   if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
 Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index 069aed148af1..370c9ac8f1de 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -156,6 +156,10 @@ VirtioRngGetRNG (
   }
 
   Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
+  if (!Dev->Ready) {
+DEBUG ((DEBUG_INFO, "%a: not ready\n", __func__));
+return EFI_DEVICE_ERROR;
+  }
   //
   // Map Buffer's system physical address to device address
   //
@@ -382,6 +386,7 @@ VirtioRngInit (
   //
   Dev->Rng.GetInfo = VirtioRngGetInfo;
   Dev->Rng.GetRNG  = VirtioRngGetRNG;
+  Dev->Ready = TRUE;
 
   return EFI_SUCCESS;
 
@@ -414,8 +419,8 @@ VirtioRngUninit (
   // VIRTIO_CFG_WRITE() returns, the host will have learned to stay away from
   // the old comms area.
   //
+  Dev->Ready = FALSE;
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-
   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
 
   VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
@@ -435,7 +440,7 @@ VirtioRngExitBoot (
 {
   VIRTIO_RNG_DEV  *Dev;
 
-  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __func__, Context));
+  DEBUG ((DEBUG_INFO, "%a: Context=0x%p\n", __func__, Context));
   //
   // Reset the device. This causes the hypervisor to forget about the virtio
   // ring.
@@ -444,6 +449,7 @@ VirtioRngExitBoot (
   // executing after ExitBootServices() is permitted to overwrite it.
   //
   Dev = Context;
+  Dev->Ready = FALSE;
  

Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

2024-05-24 Thread Gerd Hoffmann
On Fri, May 24, 2024 at 11:41:04AM GMT, Ard Biesheuvel wrote:
> On Fri, 24 May 2024 at 11:12, gaoliming via groups.io
>  wrote:
> >
> > Ard:
> >   Here is Doug PR https://github.com/tianocore/edk2/pull/5582 that includes 
> > 20 commits. You can check them.
> >
> 
> This looks fine to me in principle.
> 
> Reviewed-by: Ard Biesheuvel 
> 
> However, IIUC, the impact of this series is that all out-of-tree
> platforms that lack the right implementation of the EFI_RNG_PROTOCOL
> (i.e., using a GUID that appears in the allowlist) will lose the
> ability to do network boot. If that is a tolerable result, I am fine
> with that too, but I think it needs to be made very clear in the
> stable tag release notes.

Tested the v3 series with OVMF, results are as expected:  Without
virtio-rng-pci network boot does not work.  With virtio-rng-pci
everything is fine.

Tested-by: Gerd Hoffmann 
Acked-by: Gerd Hoffmann 

Agree that this must be noted in the release notes.

Related: I'm working on patch series adding RngDxe to OVMF with
runtime rdrand detection:
https://github.com/kraxel/edk2/commits/devel/ovmf-rdrand/

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119254): https://edk2.groups.io/g/devel/message/119254
Mute This Topic: https://groups.io/mt/106278600/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

2024-05-24 Thread Ard Biesheuvel
On Fri, 24 May 2024 at 11:12, gaoliming via groups.io
 wrote:
>
> Ard:
>   Here is Doug PR https://github.com/tianocore/edk2/pull/5582 that includes 
> 20 commits. You can check them.
>

This looks fine to me in principle.

Reviewed-by: Ard Biesheuvel 

However, IIUC, the impact of this series is that all out-of-tree
platforms that lack the right implementation of the EFI_RNG_PROTOCOL
(i.e., using a GUID that appears in the allowlist) will lose the
ability to do network boot. If that is a tolerable result, I am fine
with that too, but I think it needs to be made very clear in the
stable tag release notes.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119253): https://edk2.groups.io/g/devel/message/119253
Mute This Topic: https://groups.io/mt/106278600/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-




Re: [edk2-devel] [PATCH v3 00/20] NetworkPkg: CVE-2023-45236 and CVE-2023-45237

2024-05-24 Thread Ard Biesheuvel
On Fri, 24 May 2024 at 09:01, gaoliming via groups.io
 wrote:
>
> Ard and Gerd:
>   Doug updated this patch set based on your suggestion. Could you give
> reviewed-by or acked-by for the changes in OvmfPkg and ArmVirtPkg if you
> have no other comments?
>

I see ~60 patches from Doug, seemingly 3 copies of the v3 series. I am
going to assume they are identical.

The changes I contributed should appear at the beginning of the
series, not the end, so that bisect does not get broken.

Perhaps Doug could send a v4 (only once!) that has everything in the
correct order, and I will look at it.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119249): https://edk2.groups.io/g/devel/message/119249
Mute This Topic: https://groups.io/mt/106277453/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-