Currently networking can only be enabled/disabled at compile time. This
patch series will add support to disable VirtIo net at runtime even if
the functionality is built into binary at compile time.

This will enable VMM to reduce attack surface without recompilation.

The changes can be seen at:
https://github.com/yyu/edk2/tree/network_cfg_lib_v1

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>

Yuan Yu (2):
  OvmfPkg: Introduce NetworkCfgLib
  OvmfPkg: Use PcdNetworkSupport to enable/disable VirtIo net

 OvmfPkg/OvmfPkg.dec                             |  3 ++
 OvmfPkg/OvmfPkgX64.dsc                          |  7 ++++-
 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf | 29 ++++++++++++++++++
 OvmfPkg/VirtioNetDxe/VirtioNet.inf              |  3 ++
 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c   | 32 ++++++++++++++++++++
 OvmfPkg/VirtioNetDxe/EntryPoint.c               | 10 ++++++
 6 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf
 create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c

-- 
2.37.1.559.g78731f0fdb-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92118): https://edk2.groups.io/g/devel/message/92118
Mute This Topic: https://groups.io/mt/92808627/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/04/22 04:52, Yuan Yu wrote:
> Currently networking can only be enabled/disabled at compile time. This
> patch series will add support to disable VirtIo net at runtime even if
> the functionality is built into binary at compile time.
> 
> This will enable VMM to reduce attack surface without recompilation.
> 
> The changes can be seen at:
> https://github.com/yyu/edk2/tree/network_cfg_lib_v1
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> 
> Yuan Yu (2):
>   OvmfPkg: Introduce NetworkCfgLib
>   OvmfPkg: Use PcdNetworkSupport to enable/disable VirtIo net
> 
>  OvmfPkg/OvmfPkg.dec                             |  3 ++
>  OvmfPkg/OvmfPkgX64.dsc                          |  7 ++++-
>  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf | 29 ++++++++++++++++++
>  OvmfPkg/VirtioNetDxe/VirtioNet.inf              |  3 ++
>  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c   | 32 ++++++++++++++++++++
>  OvmfPkg/VirtioNetDxe/EntryPoint.c               | 10 ++++++
>  6 files changed, 83 insertions(+), 1 deletion(-)
>  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf
>  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c
> 

Well I've not been reviewing upstream edk2 patches for a while, but the
virtio-net driver is still very close to my heart, so this patch kind of
hits a nerve.

I think I disagree with the idea and the implementation both.

Minimally, the idea needs a much better elaboration -- what is the
threat model? Do you want to protect the host from the guest, or the
guest from the host? Or something else? How does controlling a single
SNP driver via fw_cfg (which is also dictated by the host) help?

Regarding the implementation: there is much more to networking in edk2
than VirtioNetDxe. UEFI driver binaries (SNP drivers) built from iPXE
can be passed in via the NICs' option ROMs. SNP drivers can be loaded
from the UEFI system partition (for example, Intel's binary-only driver
for QEMU's e1000* cards).

If you can control this fw_cfg switch from the VMM side, you can also
control the VMM enough to simply *not give* a virtio-net device to the
guest. Then the driver (it being a UEFI driver following the UEFI driver
model) will simply not have anything to bind.

Sorry I find this approach very wrong. If you really need it for your
particular VMM, I kind of suggest not upstreaming this patch. I see it
as a step backwards for the upstream project.

Anyway: I emphasize I'm no longer a reviewer for OvmfPkg, and it's quite
exceptional that I'm reacting at all to being CC'd on this.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92102): https://edk2.groups.io/g/devel/message/92102
Mute This Topic: https://groups.io/mt/92808627/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 4 Aug 2022 at 07:55, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 08/04/22 04:52, Yuan Yu wrote:
> > Currently networking can only be enabled/disabled at compile time. This
> > patch series will add support to disable VirtIo net at runtime even if
> > the functionality is built into binary at compile time.
> >
> > This will enable VMM to reduce attack surface without recompilation.
> >
> > The changes can be seen at:
> > https://github.com/yyu/edk2/tree/network_cfg_lib_v1
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Julien Grall <julien@xen.org>
> >
> > Yuan Yu (2):
> >   OvmfPkg: Introduce NetworkCfgLib
> >   OvmfPkg: Use PcdNetworkSupport to enable/disable VirtIo net
> >
> >  OvmfPkg/OvmfPkg.dec                             |  3 ++
> >  OvmfPkg/OvmfPkgX64.dsc                          |  7 ++++-
> >  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf | 29 ++++++++++++++++++
> >  OvmfPkg/VirtioNetDxe/VirtioNet.inf              |  3 ++
> >  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c   | 32 ++++++++++++++++++++
> >  OvmfPkg/VirtioNetDxe/EntryPoint.c               | 10 ++++++
> >  6 files changed, 83 insertions(+), 1 deletion(-)
> >  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf
> >  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c
> >
>
> Well I've not been reviewing upstream edk2 patches for a while, but the
> virtio-net driver is still very close to my heart, so this patch kind of
> hits a nerve.
>

Welcome back old friend!

> I think I disagree with the idea and the implementation both.
>
> Minimally, the idea needs a much better elaboration -- what is the
> threat model? Do you want to protect the host from the guest, or the
> guest from the host? Or something else? How does controlling a single
> SNP driver via fw_cfg (which is also dictated by the host) help?
>

I have to confess that I was the one who suggested this approach to
Yuan internally, but mainly to get the discussion going, as I was
anticipating some pushback, just not from you :-)

'Reducing the attack surface' is probably not the most accurate
characterization of the purpose. We are simply looking for a way to
disable network boot from the vmm/host side without affecting
how/which network interfaces the guest exposes to the OS.

> Regarding the implementation: there is much more to networking in edk2
> than VirtioNetDxe. UEFI driver binaries (SNP drivers) built from iPXE
> can be passed in via the NICs' option ROMs. SNP drivers can be loaded
> from the UEFI system partition (for example, Intel's binary-only driver
> for QEMU's e1000* cards).
>
> If you can control this fw_cfg switch from the VMM side, you can also
> control the VMM enough to simply *not give* a virtio-net device to the
> guest. Then the driver (it being a UEFI driver following the UEFI driver
> model) will simply not have anything to bind.
>

Sure, but then the OS will lose networking as well. We just want to
remove the ability to network boot without impacting anything else
that relies on virtio-net

> Sorry I find this approach very wrong. If you really need it for your
> particular VMM, I kind of suggest not upstreaming this patch. I see it
> as a step backwards for the upstream project.
>

If there are better ways to achieve this, we're all ears, but I think
that having a PCD which could either be fixed at build and compiled
out completely, or be set via a NULL library resolution, or even be
wired to a menu option (using PcdsDynamicHii] is a rather low-impact
but flexible way to go about this.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92108): https://edk2.groups.io/g/devel/message/92108
Mute This Topic: https://groups.io/mt/92808627/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/04/22 11:58, Ard Biesheuvel wrote:
> On Thu, 4 Aug 2022 at 07:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 08/04/22 04:52, Yuan Yu wrote:
>>> Currently networking can only be enabled/disabled at compile time. This
>>> patch series will add support to disable VirtIo net at runtime even if
>>> the functionality is built into binary at compile time.
>>>
>>> This will enable VMM to reduce attack surface without recompilation.
>>>
>>> The changes can be seen at:
>>> https://github.com/yyu/edk2/tree/network_cfg_lib_v1
>>>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Julien Grall <julien@xen.org>
>>>
>>> Yuan Yu (2):
>>>   OvmfPkg: Introduce NetworkCfgLib
>>>   OvmfPkg: Use PcdNetworkSupport to enable/disable VirtIo net
>>>
>>>  OvmfPkg/OvmfPkg.dec                             |  3 ++
>>>  OvmfPkg/OvmfPkgX64.dsc                          |  7 ++++-
>>>  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf | 29 ++++++++++++++++++
>>>  OvmfPkg/VirtioNetDxe/VirtioNet.inf              |  3 ++
>>>  OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c   | 32 ++++++++++++++++++++
>>>  OvmfPkg/VirtioNetDxe/EntryPoint.c               | 10 ++++++
>>>  6 files changed, 83 insertions(+), 1 deletion(-)
>>>  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.inf
>>>  create mode 100644 OvmfPkg/Library/NetworkCfgLib/NetworkCfgLib.c
>>>
>>
>> Well I've not been reviewing upstream edk2 patches for a while, but the
>> virtio-net driver is still very close to my heart, so this patch kind of
>> hits a nerve.
>>
> 
> Welcome back old friend!
> 
>> I think I disagree with the idea and the implementation both.
>>
>> Minimally, the idea needs a much better elaboration -- what is the
>> threat model? Do you want to protect the host from the guest, or the
>> guest from the host? Or something else? How does controlling a single
>> SNP driver via fw_cfg (which is also dictated by the host) help?
>>
> 
> I have to confess that I was the one who suggested this approach to
> Yuan internally, but mainly to get the discussion going, as I was
> anticipating some pushback, just not from you :-)

Heh, sorry about that :)

> 
> 'Reducing the attack surface' is probably not the most accurate
> characterization of the purpose. We are simply looking for a way to
> disable network boot from the vmm/host side without affecting
> how/which network interfaces the guest exposes to the OS.
> 
>> Regarding the implementation: there is much more to networking in edk2
>> than VirtioNetDxe. UEFI driver binaries (SNP drivers) built from iPXE
>> can be passed in via the NICs' option ROMs. SNP drivers can be loaded
>> from the UEFI system partition (for example, Intel's binary-only driver
>> for QEMU's e1000* cards).
>>
>> If you can control this fw_cfg switch from the VMM side, you can also
>> control the VMM enough to simply *not give* a virtio-net device to the
>> guest. Then the driver (it being a UEFI driver following the UEFI driver
>> model) will simply not have anything to bind.
>>
> 
> Sure, but then the OS will lose networking as well. We just want to
> remove the ability to network boot without impacting anything else
> that relies on virtio-net
> 
>> Sorry I find this approach very wrong. If you really need it for your
>> particular VMM, I kind of suggest not upstreaming this patch. I see it
>> as a step backwards for the upstream project.
>>
> 
> If there are better ways to achieve this, we're all ears, but I think
> that having a PCD which could either be fixed at build and compiled
> out completely, or be set via a NULL library resolution, or even be
> wired to a menu option (using PcdsDynamicHii] is a rather low-impact
> but flexible way to go about this.

How about using PCDs, but at a higher level in the edk2 network stack
(regardless of the SNP driver(s) used)?

Not that I'm a huge fan of them, but we already have PcdIPv4PXESupport
and PcdIPv6PXESupport, and they can be set for OVMF and ArmVirtQemu via
fw_cfg. If both PCDs are "PXE_DISABLED" (0), then
PxeBcDriverEntryPoint() [NetworkPkg/UefiPxeBcDxe/PxeBcDriver.c] exits
early. See:

- https://bugzilla.tianocore.org/show_bug.cgi?id=1695
- https://bugzilla.tianocore.org/show_bug.cgi?id=2681

So that's at least "prior art".

If similar PCDs could be introduced for other kinds of network boot (I
think there's only HTTP(S)v[46] to speak of [*]), i.e. in
"NetworkPkg.dec", then I guess setting those from fw_cfg in ArmVirtQemu
and OVMF could be fine. I assume HttpBootDxe would be the driver to
short-circuit.

[*] Hmm, maybe not just that. Do we consider booting off an iSCSI device
"network boot"? Perhaps where you want to "cut the stack" is
Ip4Dxe/Ip6Dxe. Prior art with PXE does suggest PCDs for the higher-level
network boot drivers.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92112): https://edk2.groups.io/g/devel/message/92112
Mute This Topic: https://groups.io/mt/92808627/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-