On Wed, Mar 01, 2017 at 07:17:10PM +0100, Reyk Floeter wrote:
> On Wed, Mar 01, 2017 at 01:04:57PM +0100, Reyk Floeter wrote:
> > Hi,
> >
> > add the new "fixed lladdr" option: when multiple VMs are connected to
> > a switch, it is desirable that an individual VM cannot spoof another
> > MAC address, especially when using meta-data*. vmd(8) can enforce
> > this by comparing the address in the Ethernet header with the
> > configured/generated address of the VM interface.
> >
> > This somewhat resembles the following features from VMware:
> > ethernet0.noforgedsrcaddr = "TRUE"
> > ethernet0.nopromisc = "TRUE"
> >
> > The important parts of the diff** are in the two if statements including
> > "dev->fixedmac" below, the rest is infrastructure, config, and
> > documentation.
> >
> > I could have used bridge(4) rules or switch(4) OpenFlow actions, but I
> > decided to implement it in vmd(8) directly to make it easier and to
> > work in all cases independent from the switch type.
> >
> > *) https://github.com/reyk/meta-data
> > **) this diff conflicts with the previous vmm.c split. Whatever goes
> > in first, I can update and resend it accordingly.
> >
> > OK?
> >
> > Reyk
> >
> > Add "fixed lladdr" option to prevent VMs from spoofing MAC addresses.
> >
> > This is especially useful when multiple VMs share a switch, the
> > implementation is independent from the underlying switch or bridge.
> >
>
> After some discussions, this is the updated diff:
>
> - change "fixed" to "locked", adjust manpage
> - sync with latest vmm.c split
>
> Reyk
>
no objections
> Index: usr.sbin/vmd/config.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.25
> diff -u -p -u -p -r1.25 config.c
> --- usr.sbin/vmd/config.c 1 Mar 2017 07:43:33 -0000 1.25
> +++ usr.sbin/vmd/config.c 1 Mar 2017 18:16:20 -0000
> @@ -236,7 +236,8 @@ config_setvm(struct privsep *ps, struct
> }
>
> /* Set the interface status */
> - vif->vif_flags = vmc->vmc_ifflags[i] & IFF_UP;
> + vif->vif_flags =
> + vmc->vmc_ifflags[i] & (VMIFF_UP|VMIFF_OPTMASK);
> }
>
> /* Open TTY */
> Index: usr.sbin/vmd/parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
> retrieving revision 1.21
> diff -u -p -u -p -r1.21 parse.y
> --- usr.sbin/vmd/parse.y 1 Mar 2017 07:43:33 -0000 1.21
> +++ usr.sbin/vmd/parse.y 1 Mar 2017 18:16:20 -0000
> @@ -116,10 +116,11 @@ typedef struct {
>
> %token INCLUDE ERROR
> %token ADD DISK DOWN GROUP INTERFACE NIFS PATH SIZE SWITCH UP VMID
> -%token ENABLE DISABLE VM KERNEL LLADDR MEMORY OWNER
> +%token ENABLE DISABLE VM KERNEL LLADDR MEMORY OWNER LOCKED
> %token <v.string> STRING
> %token <v.number> NUMBER
> %type <v.number> disable
> +%type <v.number> locked
> %type <v.number> updown
> %type <v.lladdr> lladdr
> %type <v.string> string
> @@ -174,7 +175,7 @@ switch : SWITCH string {
>
> vsw->sw_id = env->vmd_nswitches + 1;
> vsw->sw_name = $2;
> - vsw->sw_flags = IFF_UP;
> + vsw->sw_flags = VMIFF_UP;
> snprintf(vsw->sw_ifname, sizeof(vsw->sw_ifname),
> "%s%u", vsw_type, vsw_unit++);
> TAILQ_INIT(&vsw->sw_ifs);
> @@ -241,11 +242,14 @@ switch_opts : disable {
> }
> free($2);
> }
> + | LOCKED LLADDR {
> + vsw->sw_flags |= VMIFF_LOCKED;
> + }
> | updown {
> if ($1)
> - vsw->sw_flags |= IFF_UP;
> + vsw->sw_flags |= VMIFF_UP;
> else
> - vsw->sw_flags &= ~IFF_UP;
> + vsw->sw_flags &= ~VMIFF_UP;
> }
> ;
>
> @@ -503,14 +507,16 @@ iface_opts : SWITCH string {
> sizeof(vmc.vmc_ifgroup[i]));
> free($2);
> }
> - | LLADDR lladdr {
> - memcpy(vcp->vcp_macs[vcp_nnics], $2, ETHER_ADDR_LEN);
> + | locked LLADDR lladdr {
> + if ($1)
> + vmc.vmc_ifflags[vcp_nnics] |= VMIFF_LOCKED;
> + memcpy(vcp->vcp_macs[vcp_nnics], $3, ETHER_ADDR_LEN);
> }
> | updown {
> if ($1)
> - vmc.vmc_ifflags[vcp_nnics] |= IFF_UP;
> + vmc.vmc_ifflags[vcp_nnics] |= VMIFF_UP;
> else
> - vmc.vmc_ifflags[vcp_nnics] &= ~IFF_UP;
> + vmc.vmc_ifflags[vcp_nnics] &= ~VMIFF_UP;
> }
> ;
>
> @@ -541,6 +547,10 @@ lladdr : STRING {
> }
> ;
>
> +locked : /* empty */ { $$ = 0; }
> + | LOCKED { $$ = 1; }
> + ;
> +
> updown : UP { $$ = 1; }
> | DOWN { $$ = 0; }
> ;
> @@ -606,6 +616,7 @@ lookup(char *s)
> { "interfaces", NIFS },
> { "kernel", KERNEL },
> { "lladdr", LLADDR },
> + { "locked", LOCKED },
> { "memory", MEMORY },
> { "owner", OWNER },
> { "size", SIZE },
> Index: usr.sbin/vmd/priv.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/priv.c,v
> retrieving revision 1.5
> diff -u -p -u -p -r1.5 priv.c
> --- usr.sbin/vmd/priv.c 29 Oct 2016 14:56:05 -0000 1.5
> +++ usr.sbin/vmd/priv.c 1 Mar 2017 18:16:20 -0000
> @@ -295,7 +295,7 @@ vm_priv_ifconfig(struct privsep *ps, str
> }
>
> /* Set the new interface status to up or down */
> - proc_compose(ps, PROC_PRIV, (vif->vif_flags & IFF_UP) ?
> + proc_compose(ps, PROC_PRIV, (vif->vif_flags & VMIFF_UP) ?
> IMSG_VMDOP_PRIV_IFUP : IMSG_VMDOP_PRIV_IFDOWN,
> &vfr, sizeof(vfr));
> }
> @@ -339,7 +339,7 @@ vm_priv_brconfig(struct privsep *ps, str
> }
>
> /* Set the new interface status to up or down */
> - proc_compose(ps, PROC_PRIV, (vsw->sw_flags & IFF_UP) ?
> + proc_compose(ps, PROC_PRIV, (vsw->sw_flags & VMIFF_UP) ?
> IMSG_VMDOP_PRIV_IFUP : IMSG_VMDOP_PRIV_IFDOWN,
> &vfr, sizeof(vfr));
>
> Index: usr.sbin/vmd/virtio.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.32
> diff -u -p -u -p -r1.32 virtio.c
> --- usr.sbin/vmd/virtio.c 13 Feb 2017 18:49:01 -0000 1.32
> +++ usr.sbin/vmd/virtio.c 1 Mar 2017 18:16:20 -0000
> @@ -897,6 +897,7 @@ vionet_rx(struct vionet_dev *dev)
> {
> char buf[PAGE_SIZE];
> int hasdata, num_enq = 0, spc = 0;
> + struct ether_header *eh;
> ssize_t sz;
>
> do {
> @@ -909,9 +910,14 @@ vionet_rx(struct vionet_dev *dev)
> if (errno != EAGAIN)
> log_warn("unexpected read error on vionet "
> "device");
> - } else if (sz != 0)
> - num_enq += vionet_enq_rx(dev, buf, sz, &spc);
> - else if (sz == 0) {
> + } else if (sz != 0) {
> + eh = (struct ether_header *)buf;
> + if (!dev->lockedmac || sz < ETHER_HDR_LEN ||
> + ETHER_IS_MULTICAST(eh->ether_dhost) ||
> + memcmp(eh->ether_dhost, dev->mac,
> + sizeof(eh->ether_dhost)) == 0)
> + num_enq += vionet_enq_rx(dev, buf, sz, &spc);
> + } else if (sz == 0) {
> log_debug("process_rx: no data");
> hasdata = 0;
> break;
> @@ -1044,6 +1050,7 @@ vionet_notifyq(struct vionet_dev *dev)
> struct vring_desc *desc, *pkt_desc, *hdr_desc;
> struct vring_avail *avail;
> struct vring_used *used;
> + struct ether_header *eh;
>
> vr = pkt = NULL;
> ret = 0;
> @@ -1152,8 +1159,16 @@ vionet_notifyq(struct vionet_dev *dev)
> goto out;
> }
>
> + /* reject other source addresses */
> + if (dev->lockedmac && pktsz >= ETHER_HDR_LEN &&
> + (eh = (struct ether_header *)pkt) &&
> + memcmp(eh->ether_shost, dev->mac,
> + sizeof(eh->ether_shost)) != 0)
> + log_debug("vionet: wrong source address %s for vm %d",
> + ether_ntoa((struct ether_addr *)
> + eh->ether_shost), dev->vm_id);
> /* XXX signed vs unsigned here, funky cast */
> - if (write(dev->fd, pkt, pktsz) != (int)pktsz) {
> + else if (write(dev->fd, pkt, pktsz) != (int)pktsz) {
> log_warnx("vionet: tx failed writing to tap: "
> "%d", errno);
> goto out;
> @@ -1298,8 +1313,9 @@ vmmci_io(int dir, uint16_t reg, uint32_t
> }
>
> void
> -virtio_init(struct vm_create_params *vcp, int *child_disks, int *child_taps)
> +virtio_init(struct vmop_create_params *vmc, int *child_disks, int
> *child_taps)
> {
> + struct vm_create_params *vcp = &vmc->vmc_params;
> static const uint8_t zero_mac[6];
> uint8_t id;
> uint8_t i;
> @@ -1458,10 +1474,13 @@ virtio_init(struct vm_create_params *vcp
> vionet[i].mac[4] = rng;
> vionet[i].mac[5] = rng >> 8;
> }
> + vionet[i].lockedmac =
> + vmc->vmc_ifflags[i] & VMIFF_LOCKED ? 1 : 0;
>
> - log_debug("%s: vm \"%s\" vio%u lladdr %s",
> + log_debug("%s: vm \"%s\" vio%u lladdr %s%s",
> __func__, vcp->vcp_name, i,
> - ether_ntoa((void *)vionet[i].mac));
> + ether_ntoa((void *)vionet[i].mac),
> + vionet[i].lockedmac ? " (locked)" : "");
> }
> }
>
> Index: usr.sbin/vmd/virtio.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v
> retrieving revision 1.9
> diff -u -p -u -p -r1.9 virtio.h
> --- usr.sbin/vmd/virtio.h 21 Jan 2017 12:45:41 -0000 1.9
> +++ usr.sbin/vmd/virtio.h 1 Mar 2017 18:16:21 -0000
> @@ -118,6 +118,7 @@ struct vionet_dev {
> uint32_t vm_id;
> int irq;
> uint8_t mac[6];
> + int lockedmac;
> };
>
> struct virtio_net_hdr {
> @@ -150,7 +151,7 @@ struct vmmci_dev {
> int irq;
> };
>
> -void virtio_init(struct vm_create_params *, int *, int *);
> +void virtio_init(struct vmop_create_params *, int *, int *);
> uint32_t vring_size(uint32_t);
>
> int virtio_rnd_io(int, uint16_t, uint32_t *, uint8_t *, void *);
> Index: usr.sbin/vmd/vm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vm.c,v
> retrieving revision 1.1
> diff -u -p -u -p -r1.1 vm.c
> --- usr.sbin/vmd/vm.c 1 Mar 2017 18:00:50 -0000 1.1
> +++ usr.sbin/vmd/vm.c 1 Mar 2017 18:16:21 -0000
> @@ -63,7 +63,7 @@
>
> io_fn_t ioports_map[MAX_PORTS];
>
> -int run_vm(int *, int *, struct vm_create_params *, struct vcpu_reg_state *);
> +int run_vm(int *, int *, struct vmop_create_params *, struct vcpu_reg_state
> *);
> void vm_dispatch_vmm(int, short, void *);
> void *event_thread(void *);
> void *vcpu_run_loop(void *);
> @@ -72,7 +72,7 @@ int vcpu_reset(uint32_t, uint32_t, struc
> void create_memory_map(struct vm_create_params *);
> int alloc_guest_mem(struct vm_create_params *);
> int vmm_create_vm(struct vm_create_params *);
> -void init_emulated_hw(struct vm_create_params *, int *, int *);
> +void init_emulated_hw(struct vmop_create_params *, int *, int *);
> void vcpu_exit_inout(struct vm_run_params *);
> uint8_t vcpu_exit_pci(struct vm_run_params *);
> int vcpu_pic_intr(uint32_t, uint32_t, uint8_t);
> @@ -236,7 +236,7 @@ start_vm(struct vmd_vm *vm, int fd)
> fatal("setup vm pipe");
>
> /* Execute the vcpu run loop(s) for this VM */
> - ret = run_vm(vm->vm_disks, nicfds, vcp, &vrs);
> + ret = run_vm(vm->vm_disks, nicfds, &vm->vm_params, &vrs);
>
> return (ret);
> }
> @@ -493,9 +493,10 @@ vmm_create_vm(struct vm_create_params *v
> * Initializes the userspace hardware emulation
> */
> void
> -init_emulated_hw(struct vm_create_params *vcp, int *child_disks,
> +init_emulated_hw(struct vmop_create_params *vmc, int *child_disks,
> int *child_taps)
> {
> + struct vm_create_params *vcp = &vmc->vmc_params;
> int i;
>
> /* Reset the IO port map */
> @@ -534,7 +535,7 @@ init_emulated_hw(struct vm_create_params
> pci_init();
>
> /* Initialize virtio devices */
> - virtio_init(vcp, child_disks, child_taps);
> + virtio_init(vmc, child_disks, child_taps);
> }
>
> /*
> @@ -545,7 +546,7 @@ init_emulated_hw(struct vm_create_params
> * Parameters:
> * child_disks: previously-opened child VM disk file file descriptors
> * child_taps: previously-opened child tap file descriptors
> - * vcp: vm_create_params struct containing the VM's desired creation
> + * vmc: vmop_create_params struct containing the VM's desired creation
> * configuration
> * vrs: VCPU register state to initialize
> *
> @@ -554,9 +555,10 @@ init_emulated_hw(struct vm_create_params
> * !0 : the VM exited abnormally or failed to start
> */
> int
> -run_vm(int *child_disks, int *child_taps, struct vm_create_params *vcp,
> +run_vm(int *child_disks, int *child_taps, struct vmop_create_params *vmc,
> struct vcpu_reg_state *vrs)
> {
> + struct vm_create_params *vcp = &vmc->vmc_params;
> uint8_t evdone = 0;
> size_t i;
> int ret;
> @@ -597,7 +599,7 @@ run_vm(int *child_disks, int *child_taps
> log_debug("%s: initializing hardware for vm %s", __func__,
> vcp->vcp_name);
>
> - init_emulated_hw(vcp, child_disks, child_taps);
> + init_emulated_hw(vmc, child_disks, child_taps);
>
> ret = pthread_mutex_init(&threadmutex, NULL);
> if (ret) {
> Index: usr.sbin/vmd/vm.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vm.conf.5,v
> retrieving revision 1.15
> diff -u -p -u -p -r1.15 vm.conf.5
> --- usr.sbin/vmd/vm.conf.5 1 Mar 2017 12:29:06 -0000 1.15
> +++ usr.sbin/vmd/vm.conf.5 1 Mar 2017 18:16:21 -0000
> @@ -130,11 +130,16 @@ rules for several VM interfaces in the s
> The
> .Ar group-name
> must not end with a digit.
> -.It Cm lladdr Ar etheraddr
> +.It Oo Cm locked Oc Cm lladdr Op Ar etheraddr
> Change the link layer address (MAC address) of the interface on the
> VM guest side.
> If not specified, a randomized address will be assigned by
> .Xr vmd 8 .
> +If the
> +.Cm locked
> +keyword is specified,
> +.Xr vmd 8
> +will drop packets from the VM with altered source addresses.
> .It Cm switch Ar name
> Set the virtual switch
> by
> @@ -221,6 +226,11 @@ This is the default if neither
> nor
> .Cm disable
> is specified.
> +.It Cm locked lladdr
> +If this option is specified,
> +.Xr vmd 8
> +will drop packets with altered sources addresses that do not match the
> +link layer addresses (MAC addresses) of the VM interfaces in this switch.
> .It Cm disable
> Do not configure this switch.
> .It Cm group Ar group-name
> Index: usr.sbin/vmd/vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.52
> diff -u -p -u -p -r1.52 vmd.c
> --- usr.sbin/vmd/vmd.c 1 Mar 2017 07:43:33 -0000 1.52
> +++ usr.sbin/vmd/vmd.c 1 Mar 2017 18:16:21 -0000
> @@ -753,6 +753,7 @@ vm_register(struct privsep *ps, struct v
> struct vmd_vm *vm = NULL;
> struct vm_create_params *vcp = &vmc->vmc_params;
> unsigned int i;
> + struct vmd_switch *sw;
>
> errno = 0;
> *ret_vm = NULL;
> @@ -801,13 +802,20 @@ vm_register(struct privsep *ps, struct v
> goto fail;
>
> memcpy(&vm->vm_params, vmc, sizeof(vm->vm_params));
> + vmc = &vm->vm_params;
> vm->vm_pid = -1;
> vm->vm_tty = -1;
>
> for (i = 0; i < vcp->vcp_ndisks; i++)
> vm->vm_disks[i] = -1;
> - for (i = 0; i < vcp->vcp_nnics; i++)
> + for (i = 0; i < vcp->vcp_nnics; i++) {
> vm->vm_ifs[i].vif_fd = -1;
> +
> + if ((sw = switch_getbyname(vmc->vmc_ifswitch[i])) != NULL) {
> + /* inherit per-interface flags from the switch */
> + vmc->vmc_ifflags[i] |= (sw->sw_flags & VMIFF_OPTMASK);
> + }
> + }
> vm->vm_kernel = -1;
> vm->vm_iev.ibuf.fd = -1;
>
> Index: usr.sbin/vmd/vmd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
> retrieving revision 1.46
> diff -u -p -u -p -r1.46 vmd.h
> --- usr.sbin/vmd/vmd.h 1 Mar 2017 18:00:50 -0000 1.46
> +++ usr.sbin/vmd/vmd.h 1 Mar 2017 18:16:21 -0000
> @@ -113,6 +113,9 @@ struct vmop_create_params {
>
> /* userland-only part of the create params */
> unsigned int vmc_ifflags[VMM_MAX_NICS_PER_VM];
> +#define VMIFF_UP 0x01
> +#define VMIFF_LOCKED 0x02
> +#define VMIFF_OPTMASK VMIFF_LOCKED
> char vmc_ifnames[VMM_MAX_NICS_PER_VM][IF_NAMESIZE];
> char vmc_ifswitch[VMM_MAX_NICS_PER_VM][VM_NAME_MAX];
> char vmc_ifgroup[VMM_MAX_NICS_PER_VM][IF_NAMESIZE];
>
