|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH (V9) 2/2] xen: Add V4V implementation
Hi,
Nice to see this series again. :) Thanks for your work cleaning it up.
At 15:43 -0400 on 28 May (1369755811), Ross Philipson wrote:
> Setup of v4v domains a domain gets created and cleanup
> when a domain die. Wire up the v4v hypercall.
>
> Include v4v internal and public headers.
>
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
I'm a bit worried about resource exhaustion attacks in this interface.
In particular:
- It looks like a malicious guest can register large numbers of rings
(like, 2^48 of them), each of which causes Xen to xmalloc bookkeeping
space (up to 2^22 bytes per ring).
- A malicious guest that has enough memory can send a very large vector
(2^31 1-byte entries), which would cause Xen to read and check 2^37
bytes of control structures before even starting the copy of the data.
At a rough guess that could be >10seconds without checking for preemption.
Maybe reduce the limits a bit (say, 32MB per hypercall, may 100 elements
per vector)?
- Something similar for V4VOP_notify, I guess, though that could perhaps
be handled by the usual pending-events check + hypercall continuations.
And now, some detailed review:
> +/*
> + * Messages on the ring are padded to 128 bits
> + * Len here refers to the exact length of the data not including the
> + * 128 bit header. The message uses
> + * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes
> + */
> +#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf)
Even though this is always used on values <2^32 and so is safe,
it would be nice to have it as (((a) + 0xf) & ~(typeof (a))0xf)
so it's clear that it doesn't truncate any high-order bits.
[...]
> +/*
> + * lock to protect the filtering rules list: v4vtable_rules
> + *
> + * The write lock is held for viptables_del and viptables_add
> + * The read lock is held for viptable_list
s/viptables/v4vtables/g/
[...]
> +static int
> +v4v_ringbuf_get_rx_ptr(struct domain *d, struct v4v_ring_info *ring_info,
> + uint32_t * rx_ptr)
> +{
> + v4v_ring_t *ringp;
> +
> + if ( ring_info->npage == 0 )
> + return -1;
> +
> + ringp = map_domain_page(mfn_x(ring_info->mfns[0]));
> +
> + v4v_dprintk("v4v_ringbuf_payload_space: mapped %p to %p\n",
> + (void *)mfn_x(ring_info->mfns[0]), ringp);
The function name is wrong there, presumably after this code was
refctored. You might add %s/__func__ to the definition of v4v_dprintk()
instead?
> + if ( !ringp )
> + return -1;
> +
> + write_atomic(rx_ptr, ringp->rx_ptr);
AIUI ringp->rx_ptr is the shared variable here, so this should be
*rx_ptr = read_atomic(ringp->rx_ptr);
Also, maybe comment in the hypercall interface that the guest should
also use atomic operations when accessing rx_ptr and tx_ptr. (Though I
should say the hypercall comments are already pretty good compared to
some things we already have!)
> + mb();
I'm not 100% convinced this barrier is needed (since we're not going to
read the 'free' memory in the ring and I don't think that writes to it
can possibly be hoisted past this read since they're gated on the
result). OTOH I'm happy to err on the side of caution. :)
> + unmap_domain_page(ringp);
> + return 0;
> +}
> +
> +uint32_t
> +v4v_ringbuf_payload_space(struct domain * d, struct v4v_ring_info *
> ring_info)
> +{
> + v4v_ring_t ring;
> + int32_t ret;
> +
> + ring.tx_ptr = ring_info->tx_ptr;
> + ring.len = ring_info->len;
> +
> + if ( v4v_ringbuf_get_rx_ptr(d, ring_info, &ring.rx_ptr) )
> + return 0;
> +
> + v4v_dprintk("v4v_ringbuf_payload_space:tx_ptr=%d rx_ptr=%d\n",
> + (int)ring.tx_ptr, (int)ring.rx_ptr);
> + if ( ring.rx_ptr == ring.tx_ptr )
> + return ring.len - sizeof(struct v4v_ring_message_header);
> +
> + ret = ring.rx_ptr - ring.tx_ptr;
> + if ( ret < 0 )
> + ret += ring.len;
> +
> + ret -= sizeof(struct v4v_ring_message_header);
> + ret -= V4V_ROUNDUP(1);
> +
> + return (ret < 0) ? 0 : ret;
Sanity check for ret > ring.len as well? I guess it's not clear what
you'd do about that.
[...]
> +static long
> +v4v_ringbuf_insertv(struct domain *d,
> + struct v4v_ring_info *ring_info,
> + v4v_ring_id_t *src_id, uint32_t message_type,
> + XEN_GUEST_HANDLE_PARAM(v4v_iov_t) iovs,
> + uint32_t niov, size_t len)
> +{
> + v4v_ring_t ring;
> + struct v4v_ring_message_header mh = { 0 };
> + int32_t sp;
> + long happy_ret;
> + int32_t ret = 0;
> + XEN_GUEST_HANDLE(uint8_t) empty_hnd = { 0 };
> +
> + ASSERT(spin_is_locked(&ring_info->lock));
> +
> + happy_ret = len;
> +
> + if ( (V4V_ROUNDUP(len) + sizeof(struct v4v_ring_message_header) ) >=
> + ring_info->len)
> + return -EMSGSIZE;
> +
> + do {
> + if ( (ret = v4v_memcpy_from_guest_ring(&ring, ring_info, 0,
> + sizeof(ring))) )
> + break;
Should this be using an atomic operation to read ring.rx_ptr instead?
That's the only field we actually use from the guest copy anyway.
> +
> + ring.tx_ptr = ring_info->tx_ptr;
> + ring.len = ring_info->len;
> +
> + v4v_dprintk("ring.tx_ptr=%d ring.rx_ptr=%d ring.len=%d
> ring_info->tx_ptr=%d\n",
> + ring.tx_ptr, ring.rx_ptr, ring.len, ring_info->tx_ptr);
> +
> + if ( ring.rx_ptr == ring.tx_ptr )
> + sp = ring_info->len;
> + else
> + {
> + sp = ring.rx_ptr - ring.tx_ptr;
> + if ( sp < 0 )
> + sp += ring.len;
> + }
> +
> + if ( (V4V_ROUNDUP(len) + sizeof(struct v4v_ring_message_header)) >=
> sp )
> + {
> + v4v_dprintk("EAGAIN\n");
> + ret = -EAGAIN;
> + break;
> + }
> +
> + mh.len = len + sizeof (struct v4v_ring_message_header);
> + mh.source.port = src_id->addr.port;
> + mh.source.domain = src_id->addr.domain;
> + mh.message_type = message_type;
> +
> + if ( (ret = v4v_memcpy_to_guest_ring(ring_info,
> + ring.tx_ptr +
> sizeof(v4v_ring_t),
> + &mh, empty_hnd,
> + sizeof(mh))) )
> + break;
OK, I think this is correct, because the tx_ptr is always 16-byte
aligned and the message header is 16 bytes long, but it's not
obvious. :) Maybe add a comment here, and a BUILD_BUG_ON() to assert
that sizeof (struct v4v_ring_message_header) <= V4V_ROUNDUP(1)?
> + ring.tx_ptr += sizeof(mh);
> + if ( ring.tx_ptr == ring_info->len )
> + ring.tx_ptr = 0;
> +
> + while ( niov-- )
> + {
> + XEN_GUEST_HANDLE_PARAM(uint8_t) bufp_hnd;
> + XEN_GUEST_HANDLE(uint8_t) buf_hnd;
> + v4v_iov_t iov;
> +
> + if ( copy_from_guest(&iov, iovs, 1) )
Hrmn. So here we re-copy the iov from the guest, but the guest might
have changed it since we looked at it last (when we calculated the total
length). That means we need to keep a running count of how much we copy,
to be sure the guest doesn't swap short iovs for long ones and make
us overrun the ring's rx_ptr.
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + bufp_hnd = guest_handle_from_ptr(iov.iov_base, uint8_t);
> + buf_hnd = guest_handle_from_param(bufp_hnd, uint8_t);
> + len = iov.iov_len;
> +
> + if ( unlikely(!guest_handle_okay(buf_hnd, len)) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + sp = ring.len - ring.tx_ptr;
> +
> + if ( len > sp )
> + {
> + ret = v4v_memcpy_to_guest_ring(ring_info,
> + ring.tx_ptr + sizeof(v4v_ring_t),
> + NULL, buf_hnd, sp);
> + if ( ret )
> + break;
> +
> + ring.tx_ptr = 0;
> + len -= sp;
> + guest_handle_add_offset(buf_hnd, sp);
> + }
> +
> + ret = v4v_memcpy_to_guest_ring(ring_info,
> + ring.tx_ptr + sizeof(v4v_ring_t),
> + NULL, buf_hnd, len);
> + if ( ret )
> + break;
> +
> + ring.tx_ptr += len;
> +
> + if ( ring.tx_ptr == ring_info->len )
> + ring.tx_ptr = 0;
> +
> + guest_handle_add_offset(iovs, 1);
> + }
> + if ( ret )
> + break;
> +
> + ring.tx_ptr = V4V_ROUNDUP(ring.tx_ptr);
> +
> + if ( ring.tx_ptr >= ring_info->len )
> + ring.tx_ptr -= ring_info->len;
> +
> + mb();
> + ring_info->tx_ptr = ring.tx_ptr;
> + if ( (ret = v4v_update_tx_ptr(ring_info, ring.tx_ptr)) )
> + break;
> + } while ( 0 );
> +
> + v4v_ring_unmap(ring_info);
> +
> + return ret ? ret : happy_ret;
> +}
[...]
> +static int
> +v4v_pending_requeue(struct v4v_ring_info *ring_info, domid_t src_id, int len)
> +{
> + struct hlist_node *node;
> + struct v4v_pending_ent *ent;
> +
> + hlist_for_each_entry(ent, node, &ring_info->pending, node)
> + {
> + if ( ent->id == src_id )
> + {
> + if ( ent->len < len )
> + ent->len = len;
Does this mean that if I call V4VOP_notify with some large space
requirement, and then again with a smaller one, I won't get a
notification until the original large amount is free? Seems like this
should always overwrite the old length instead, and notify on the most
recently requested amount.
> +static int
> +v4v_fill_ring_data(struct domain *src_d,
> + XEN_GUEST_HANDLE(v4v_ring_data_ent_t) data_ent_hnd)
> +{
> + v4v_ring_data_ent_t ent;
> + struct domain *dst_d;
> + struct v4v_ring_info *ring_info;
> +
> + if ( copy_from_guest(&ent, data_ent_hnd, 1) )
> + {
> + v4v_dprintk("EFAULT\n");
> + return -EFAULT;
> + }
> +
> + v4v_dprintk("v4v_fill_ring_data: ent.ring.domain=%d,ent.ring.port=%u\n",
> + (int)ent.ring.domain, (int)ent.ring.port);
> +
> + ent.flags = 0;
> +
> + dst_d = get_domain_by_id(ent.ring.domain);
> +
> + if ( dst_d && dst_d->v4v )
> + {
> + read_lock(&dst_d->v4v->lock);
> + ring_info = v4v_ring_find_info_by_addr(dst_d, &ent.ring,
> + src_d->domain_id);
> +
Should there be a call to v4vtables_check() here? Otherwise we leak a
little bit of information to a guest who's not supposed to see the ring,
(and also it's a bit odd to say 'yes, you may send' and then bounce the
sendv call).
[...]
> +static void v4v_ring_remove_mfns(struct domain *d, struct v4v_ring_info
> *ring_info)
> +{
> + int i;
> +
> + ASSERT(rw_is_write_locked(&d->v4v->lock));
> +
> + if ( ring_info->mfns )
> + {
> + for ( i = 0; i < ring_info->npage; ++i )
> + if ( mfn_x(ring_info->mfns[i]) != 0 )
Here, and where the array is populated, it would be better to use
INVALID_MFN in any empty slots. On x86, MFN 0 is never allocated to a
guest, but that may not always be true on all architectures.
[...]
> +static long
> +v4v_ring_add(struct domain *d, XEN_GUEST_HANDLE_PARAM(v4v_ring_t) ring_hnd,
> + uint32_t npage, XEN_GUEST_HANDLE_PARAM(v4v_pfn_t) pfn_hnd)
> +{
[...]
> + /*
> + * no need for a lock yet, because only we know about this
> + * set the tx pointer if it looks bogus (we don't reset it
> + * because this might be a re-register after S4)
> + */
> + if ( (ring.tx_ptr >= ring.len)
> + || (V4V_ROUNDUP(ring.tx_ptr) != ring.tx_ptr) )
> + {
> + ring.tx_ptr = ring.rx_ptr;
> + }
Maybe check here that the guest-supplied rx_ptr is correctly aligned?
I don't think an unaligned one would be a security risk, but it might
cause entertaining breakage on the first sendv().
> + copy_field_to_guest(ring_hnd, &ring, tx_ptr);
> +
> + read_lock(&d->v4v->lock);
> + ring_info = v4v_ring_find_info(d, &ring.id);
> +
> + if ( !ring_info )
> + {
> + read_unlock(&d->v4v->lock);
> + ring_info = xmalloc(struct v4v_ring_info);
> + if ( !ring_info )
> + {
> + v4v_dprintk("ENOMEM\n");
> + ret = -ENOMEM;
> + break;
> + }
> + need_to_insert++;
> + spin_lock_init(&ring_info->lock);
> + INIT_HLIST_HEAD(&ring_info->pending);
> + ring_info->mfns = NULL;
> + }
> + else
> + {
> + /*
> + * Ring info already existed.
> + */
> + printk(KERN_INFO "v4v: dom%d ring already registered\n",
> + current->domain->domain_id);
> + ret = -EEXIST;
> + break;
> + }
If you reshuffle these so that the already-exists case is first, then
the not-already-exists case doesn't need to be indented, i.e.
if ( ring_info )
{
printk();
ret = -EEXIST;
break;
}
/* Normal case goes here */
Then I think it becomes clear that you don't need a 'need_to_insert'
flag -- and that the read_unlock() below is dead code, so the error path
doesn't unlock at all!
> +
> + spin_lock(&ring_info->lock);
> + ring_info->id = ring.id;
> + ring_info->len = ring.len;
> + ring_info->tx_ptr = ring.tx_ptr;
> + ring_info->ring = ring_hnd;
I can't see where this field is used, but AFAICT the ring_hnd isn't
guaranteed to be valid after this hypercall finishes (and certainly not
after, say, the next context switch), so storing it doesn't seem useful.
> + if ( ring_info->mfns )
> + xfree(ring_info->mfns);
> + ret = v4v_find_ring_mfns(d, ring_info, npage, pfn_hnd);
> + spin_unlock(&ring_info->lock);
> + if ( ret )
> + break;
> +
> + if ( !need_to_insert )
> + {
> + read_unlock(&d->v4v->lock);
> + }
> + else
> + {
> + uint16_t hash = v4v_hash_fn(&ring.id);
> +
> + write_lock(&d->v4v->lock);
> + hlist_add_head(&ring_info->node, &d->v4v->ring_hash[hash]);
> + write_unlock(&d->v4v->lock);
> + }
> + } while ( 0 );
> +
> + read_unlock(&v4v_lock);
> + return ret;
> +}
[...]
> +void
> +v4vtables_print_rule(struct v4vtables_rule_node *node)
> +{
[...]
> + if ( rule->accept == 1 )
> + printk("ACCEPT");
The logic in the actual ACL is (effectively) 'if ( rule->accept != 0 )';
so this probably ought to follow suit. :)
[...]
> +int
> +v4vtables_del(struct domain *src_d,
> + XEN_GUEST_HANDLE_PARAM(v4vtables_rule_t) rule_hnd,
> + int32_t position)
> +{
> + struct list_head *tmp = NULL;
> + struct list_head *to_delete = NULL;
> + struct list_head *next = NULL;
> + struct v4vtables_rule_node *node;
> +
> + ASSERT(rw_is_write_locked(&v4vtables_rules_lock));
> +
> + v4v_dprintk("v4vtables_del position:%d\n", position);
> +
> + if ( position != -1 )
> + {
> + /* We want to delete the rule number <position> */
> + list_for_each(tmp, &v4vtables_rules)
> + {
> + to_delete = tmp;
> + if (position == 0)
> + break;
> + position--;
> + }
Hmmm. If this is called with position = 1 with a multi-element list, it
selects the second element (which conflicts with the semantics of the
add operation, where '1' is the first element). And if it's called with
'1' on a single-element list, it selects the first (and only) element.
> + /* Can't find the position */
> + if (position != 0)
> + to_delete = NULL;
> + }
> + else if ( !guest_handle_is_null(rule_hnd) )
> + {
> + struct v4vtables_rule r;
> +
> + if ( copy_from_guest(&r, rule_hnd, 1) )
> + return -EFAULT;
> +
> + list_for_each(tmp, &v4vtables_rules)
> + {
> + node = list_entry(tmp, struct v4vtables_rule_node, list);
> +
> + if ( (node->rule.src.domain == r.src.domain) &&
> + (node->rule.src.port == r.src.port) &&
> + (node->rule.dst.domain == r.dst.domain) &&
> + (node->rule.dst.port == r.dst.port))
Not matching by 'accept' too? That should be mentioned in the hypercall
interface, I think.
> + {
> + to_delete = tmp;
> + break;
> + }
> + }
> + }
> + else
> + {
> + /* We want to flush the rules! */
> + printk(KERN_ERR "VIPTables: flushing rules\n");
> + list_for_each_safe(tmp, next, &v4vtables_rules)
> + {
> + node = list_entry(tmp, struct v4vtables_rule_node, list);
> + list_del(tmp);
> + xfree(node);
> + }
> + }
> +
> + if ( to_delete )
> + {
> + node = list_entry(to_delete, struct v4vtables_rule_node, list);
> +#ifdef V4V_DEBUG
> + printk(KERN_ERR "VIPTables: deleting rule: ");
> + v4vtables_print_rule(node);
> +#endif /* V4V_DEBUG */
> + list_del(to_delete);
> + xfree(node);
> + }
> +
> + return 0;
> +}
> +
> +static size_t
> +v4vtables_list(struct domain *src_d,
> + XEN_GUEST_HANDLE_PARAM(v4vtables_list_t) list_hnd)
> +{
> + struct list_head *ptr;
> + struct v4vtables_rule_node *node;
> + struct v4vtables_list rules_list;
> + uint32_t nbrules;
> + XEN_GUEST_HANDLE(v4vtables_rule_t) guest_rules;
> +
> + ASSERT(rw_is_locked(&v4vtables_rules_lock));
> +
> + memset(&rules_list, 0, sizeof (rules_list));
> + if ( copy_from_guest(&rules_list, list_hnd, 1) )
> + return -EFAULT;
> +
> + ptr = v4vtables_rules.next;
> + while ( rules_list.start_rule != 0 && ptr->next != &v4vtables_rules )
> + {
> + ptr = ptr->next;
> + rules_list.start_rule--;
> + }
Again, that seems to conflict with the 'add' semantics that the first
rule is rule 1.
And what if the supplied start_rule was too large? As it stands I think
this will return the last rule in the list rather than returning no
rules (or an error).
> +
> + if ( rules_list.nb_rules == 0 )
> + return -EINVAL;
> +
> + guest_rules = guest_handle_for_field(list_hnd, v4vtables_rule_t,
> rules[0]);
> +
> + nbrules = 0;
> + while ( nbrules < rules_list.nb_rules && ptr != &v4vtables_rules )
> + {
> + node = list_entry(ptr, struct v4vtables_rule_node, list);
> +
> + if ( copy_to_guest(guest_rules, &node->rule, 1) )
> + break;
Maybe also arrange to return -EFAULT?
> +
> + guest_handle_add_offset(guest_rules, 1);
> +
> + nbrules++;
> + ptr = ptr->next;
> + }
> +
> + rules_list.nb_rules = nbrules;
> + if ( copy_field_to_guest(list_hnd, &rules_list, nb_rules) )
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static size_t
> +v4vtables_check(v4v_addr_t *src, v4v_addr_t *dst)
> +{
> + struct list_head *ptr;
> + struct v4vtables_rule_node *node;
> + size_t ret = 0; /* Defaulting to ACCEPT */
size_t? bool_t would be better. Also, having this function return the
inverse of the rule encoding is just perverse.
[...]
> + case V4VOP_tables_add:
> + {
> + uint32_t position = arg3;
> +
> + XEN_GUEST_HANDLE_PARAM(v4vtables_rule_t) rule_hnd =
> + guest_handle_cast(arg1, v4vtables_rule_t);
> + rc = -EPERM;
> + if ( !is_hardware_domain(d) )
> + goto out;
I think this ought to be an XSM check now, with XSM_PRIV to indicate
that the default policy is to check IS_PRIV(). (Likewise for the
other table ops.)
[...]
> +struct keyhandler v4v_info_keyhandler =
> +{
> + .diagnostic = 1,
> + .u.fn = dump_state,
> + .desc = "dump v4v states and interupt"
s/interupt/interrupt/
[...]
> +/*
> + * V4VOP_register_ring
> + *
> + * Registers a ring with Xen. If a ring with the same v4v_ring_id exists,
> + * the hypercall will return -EEXIST.
> + *
> + * do_v4v_op(V4VOP_register_ring,
> + * XEN_GUEST_HANDLE(v4v_ring_t), XEN_GUEST_HANDLE(v4v_pfn_t),
> + * npage, 0)
Maybe add a 'uint32_t ' before npage, to match the same style below?
[...]
> +/*
> + * V4VOP_sendv
> + *
> + * Sends of list of buffer contained in iov.
> + *
> + * For each iov entry send iov_len bytes of iov_base to addr.dst, giving
> + * src as the source address (xen will ignore src->domain and put your
> + * domain in the actually message), xen first looks for a ring with
> id.addr==dst
> + * and id.partner==sending_domain if that fails it looks for id.addr==dst and
These lines are longer than 80 chars - can you wrap a little narrower please?
Also s/actually/actual/.
> + * id.partner==DOMID_ANY.
> + * message_type is the 32 bit number used from the message
> + * most likely V4V_MESSAGE_DGRAM or V4V_MESSAGE_STREAM. If insufficient
> space exists
> + * it will return -EAGAIN and xen will twing the V4V_INTERRUPT when
'twing'? :)
[...]
> +/*
> + * V4VOP_tables_add
> + *
> + * Insert a filtering rules after a given position.
> + *
Can you add a description of the semantics of the rules?
E.g. does the first match or the last match or the tightest match dominate?
> + * do_v4v_op(V4VOP_tables_add,
> + * XEN_GUEST_HANDLE(v4vtables_rule_t) rule,
> + * NULL,
> + * uint32_t position, 0)
> + */
> +#define V4VOP_tables_add 7
> +
> +/*
> + * V4VOP_tables_del
> + *
> + * Delete a filtering rules at a position or the rule
> + * that matches "rule".
> + *
> + * do_v4v_op(V4VOP_tables_del,
> + * XEN_GUEST_HANDLE(v4vtables_rule_t) rule,
> + * NULL,
> + * uint32_t position, 0)
> + */
> +#define V4VOP_tables_del 8
Can you describe the calling convention a bit more fully? In particular
that position should == -1 if you want to match by rule (though I think
== 0 would make more sense), and that position == -1 && rule == NULL
means to flush everything.
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |