Re: [Qemu-devel] [PATCH] mark nic as trusted

[Mark McLoughlin]

Hi Gleb,

On Wed, 2009-01-07 at 16:26 +0200, Gleb Natapov wrote:

> This patch allows to mark specific nic as trusted by adding special
> PCI capability. "Trusted" means that it is used for communication
> between host and guest and no malicious entity can inject traffic
> to the nic.

I'm not sure I follow - is this cookie a shared secret that only the
host and guest knows, or do literally mean that the cookie will contain
the string "Trusted" as a indicator that the guest can trust the NIC?

> Signed-off-by: Gleb Natapov <address@hidden>
...
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 1f45b2d..186c6bd 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -309,6 +309,9 @@ PCIDevice *virtio_net_init(PCIBus *bus, NICInfo *nd, int 
> devfn)
>      if (!n)
>          return NULL;
>  
> +    if (nd->secure_cookie[0])
> +        pci_add_capability(&n->vdev.pci_dev, 0x0f, 0xf0, nd->secure_cookie, 
> 14);

How was the Capability ID 0x0f chosen? It it unallocated by the PCI SIG
allocated it or ...? I see it's not defined in the kernel sources:

#define  PCI_CAP_ID_AGP3        0x0E    /* AGP Target PCI-PCI bridge */
#define  PCI_CAP_ID_EXP         0x10    /* PCI Express */

Also, to reduce magic numbers it would be nice to define the CAP_ID
(0xf) and offset (0xf0) as a macro somewhere and use
sizeof(nd->secure_cookie) instead of 14.

> diff --git a/net.c b/net.c
> index 6af4255..000768f 100644
> --- a/net.c
> +++ b/net.c
> @@ -1474,6 +1474,11 @@ int net_client_init(const char *device, const char *p)
>          if (get_param_value(buf, sizeof(buf), "model", p)) {
>              nd->model = strdup(buf);
>          }
> +        if (get_param_value(buf, sizeof(buf), "trusted", p)) {
> +            strncpy(nd->secure_cookie, buf, sizeof(nd->secure_cookie));
> +        } else {
> +            nd->secure_cookie[0] = NULL;

NULL isn't a uint8_t, use '\0' instead I guess. Or maybe just memset()
the NICInfo struct before starting to assign to it.

Cheers,
Mark.