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

[Gleb Natapov <gleb@redhat.com>]

On Wed, Jan 07, 2009 at 03:04:31PM +0000, Mark McLoughlin wrote:
> 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?
> 
The presence of the capability indicates that the nic is trusted, but I
added a possibility to pass 14 byte string from a host too. The string can
be used as shared secret.

> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ...
> > 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 */
> 
It is "secure device capability", so I used it based on the name.

> 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.
OK. Good point.

> 
> > 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.
> 
OK. I wounder why I've used NULL here in the first place.

--
			Gleb.