tim at xen
Jul 5, 2012, 7:23 AM
Post #2 of 2
(41 views)
Permalink
|
|
Re: [PATCH 5/5] v4v: Introduce basic access control to V4V
[In reply to]
|
|
Hi,
At 17:26 +0100 on 28 Jun (1340904386), Jean Guyader wrote:
> +#ifdef V4V_DEBUG
> +void
> +v4v_viptables_print_rule (struct v4v_viptables_rule_node *rule)
> +{
> + if (rule == NULL)
> + {
> + printk("(null)\n");
> + return;
> + }
The indentation doesn't follow the coding style at all in this patch.
> +int
> +v4v_viptables_add (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_rule_t) rule,
> + int32_t position)
> +{
> + struct v4v_viptables_rule_node* new;
> + struct list_head* tmp;
> +
> + /* First rule is n.1 */
> + position--;
> +
> + new = xmalloc (struct v4v_viptables_rule_node);
What if xmalloc() fails?
> + if (copy_field_from_guest (new, rule, src))
> + return -EFAULT;
> + if (copy_field_from_guest (new, rule, dst))
> + return -EFAULT;
> + if (copy_field_from_guest (new, rule, accept))
> + return -EFAULT;
Leaking 'new' here.
> +#ifdef V4V_DEBUG
> + printk(KERN_ERR "VIPTables: ");
> + v4v_viptables_print_rule(new);
> +#endif /* V4V_DEBUG */
> +
> + tmp = &viprules;
> + while (position != 0 && tmp->next != &viprules)
> + {
> + tmp = tmp->next;
> + position--;
> + }
> + list_add(&new->list, tmp);
Doesn't this need to be protected by a lock? AFAICS this function is
called under domain_lock(d) but modifies a global shared list, and the
readers don't take any locks. If you expect table updates to be rare
then maybe write-locking the L1 lock would suffice.
> +
> + return 0;
> +}
> +
> +int
> +v4v_viptables_del (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_rule_t) rule,
> + int32_t position)
> +{
> + struct list_head *tmp = NULL;
> + struct list_head *next = NULL;
> + struct v4v_viptables_rule_node *node;
> + struct v4v_viptables_rule *r;
> +
> + if (position != -1)
> + {
> + /* We want to delete the rule number <position> */
> + tmp = &viprules;
> + while (position != 0 && tmp->next != &viprules)
> + {
> + tmp = tmp->next;
> + position--;
> + }
> + }
> + else if (!guest_handle_is_null(rule))
> + {
> + /* We want to delete the rule <rule> */
> + r = xmalloc (struct v4v_viptables_rule);
It's probably OK for this to go on the stack - it's not that big, and...
> + if (copy_field_from_guest (r, rule, src))
> + return -EFAULT;
> + if (copy_field_from_guest (r, rule, dst))
> + return -EFAULT;
> + if (copy_field_from_guest (r, rule, accept))
> + return -EFAULT;
... it would stop you leaking 'r' here.
> + list_for_each(tmp, &viprules)
> + {
> + node = list_entry(tmp, struct v4v_viptables_rule_node, list);
> +
> + if ((node->src.domain == r->src.domain) &&
> + (node->src.port == r->src.port) &&
> + (node->dst.domain == r->dst.domain) &&
> + (node->dst.port == r->dst.port))
> + {
> + position = 0;
> + break;
> + }
> + }
> + xfree(r);
> + }
> + else
> + {
> + /* We want to flush the rules! */
> + printk(KERN_ERR "VIPTables: flushing rules\n");
> + list_for_each_safe(tmp, next, &viprules)
> + {
> + node = list_entry(tmp, struct v4v_viptables_rule_node, list);
> + list_del(tmp);
> + xfree(node);
> + }
> + }
> +
> +#ifdef V4V_DEBUG
> + if (position == 0 && tmp != &viprules)
> + {
> + printk(KERN_ERR "VIPTables: deleting rule: ");
> + node = list_entry(tmp, struct v4v_viptables_rule_node, list);
> + v4v_viptables_print_rule(node);
> + list_del(tmp);
> + xfree(node);
This list_del/xfree should definitely not be #ifdef V4V_DEBUG :)
> + }
> +#endif /* V4V_DEBUG */
> +
> + return 0;
> +}
> +
> +static size_t
> +v4v_viptables_list (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_list_t) list_hnd)
> +{
> + struct list_head *ptr;
> + struct v4v_viptables_rule_node *node;
> + struct v4v_viptables_list rules_list;
> + uint32_t nbrules;
> +
> + memset(&rules_list, 0, sizeof (rules_list));
> + if (copy_field_from_guest (&rules_list, list_hnd, nb_rules))
> + return -EFAULT;
> +
> + ptr = viprules.next;
> + while (rules_list.nb_rules != 0 && ptr->next != &viprules)
> + {
> + ptr = ptr->next;
> + rules_list.start_rule--;
> + }
> +
> + if (rules_list.nb_rules != 0)
> + return -EFAULT;
Surely s/nb_rules/start_rule/ in both the while() and the if() above?
How much testing has this had? It seems like this function could never
get as far as copying the rules out.
> +
> + nbrules = 0;
> + while (nbrules < rules_list.nb_rules && ptr != &viprules)
> + {
> + node = list_entry(ptr, struct v4v_viptables_rule_node, list);
> +
> + rules_list.rules[rules_list.nb_rules].src = node->src;
> + rules_list.rules[rules_list.nb_rules].dst = node->dst;
> + rules_list.rules[rules_list.nb_rules].accept = node->accept;
Aiee! Good thing it never gets that far, because (a) you're indirecting
a user-supplied distance into a stack array, and (b) the stack array has
zero length.
> +
> + nbrules++;
> + ptr = ptr->next;
> + }
> +
> + if (copy_to_guest(list_hnd, &rules_list, 1))
> + return -EFAULT;
And at the end you only copy the header back to the caller. :|
> +
> + return 0;
> +}
> +
> +static size_t
> +v4v_viptables_check (v4v_addr_t * src, v4v_addr_t * dst)
> +{
> + struct list_head *ptr;
> + struct v4v_viptables_rule_node *node;
> +
> + list_for_each(ptr, &viprules)
> + {
> + node = list_entry(ptr, struct v4v_viptables_rule_node, list);
> +
> + if ((node->src.domain == DOMID_INVALID || node->src.domain == src->domain) &&
Having defined a new magic V4V_DOMID_* to avoid using DOMID_INVALID, you
should probably use it here.
(oh, and while I'm here please keep lines < 80 characters).
> + (node->src.port == -1 || node->src.port == src->port) &&
Likewise, why is this not V4V_PORT_NONE?
> + (node->dst.domain == DOMID_INVALID || node->dst.domain == dst->domain) &&
> + (node->dst.port == -1 || node->dst.port == dst->port))
And what about protocol? Protocol seems to have ended up as a bit of a
second-class citizen in v4v; it's defined, and indeed required, but not
used for routing or for acccess control, so all traffic to a given port
_on every protocol_ ends up on the same ring.
This is the inverse of the TCP/IP namespace that you're copying, where
protocol demux happens before port demux. And I think it will bite
someone if you ever, for example, want to send ICMP or GRE over a v4v
channel.
> + return !node->accept;
> + }
> +
> + /* Defaulting to ACCEPT */
> + return 0;
> +}
>
> /*Hypercall to do the send*/
> static size_t
> @@ -1351,6 +1566,15 @@ v4v_send (struct domain *src_d, v4v_addr_t * src_addr,
> return -ECONNREFUSED;
> }
>
> + if (v4v_viptables_check(src_addr, dst_addr) != 0)
> + {
> + read_unlock (&v4v_lock);
> + printk(KERN_ERR "V4V: VIPTables REJECTED %i:%i -> %i:%i\n",
> + src_addr->domain, src_addr->port,
> + dst_addr->domain, dst_addr->port);
This should be at most a gdprintk to stop a badly behaved VM from
spamming the console.
> + return -ECONNREFUSED;
> + }
> +
> do
> {
> if ( !dst_d->v4v )
> @@ -1437,6 +1661,15 @@ v4v_sendv (struct domain *src_d, v4v_addr_t * src_addr,
> return -ECONNREFUSED;
> }
>
> + if (v4v_viptables_check(src_addr, dst_addr) != 0)
> + {
> + read_unlock (&v4v_lock);
> + printk(KERN_ERR "V4V: VIPTables REJECTED %i:%i -> %i:%i\n",
> + src_addr->domain, src_addr->port,
> + dst_addr->domain, dst_addr->port);
Likewise.
> + return -ECONNREFUSED;
> + }
> +
> do
> {
> if ( !dst_d->v4v )
> @@ -1596,6 +1829,38 @@ do_v4v_op (int cmd, XEN_GUEST_HANDLE (void) arg1,
> rc = v4v_notify (d, ring_data_hnd);
> break;
> }
> + case V4VOP_viptables_add:
> + {
> + uint32_t position = arg4;
> + XEN_GUEST_HANDLE (v4v_viptables_rule_t) rule_hnd =
> + guest_handle_cast (arg1, v4v_viptables_rule_t);
> + rc = -EPERM;
> + if (!IS_PRIV(d))
> + goto out;
> + rc = v4v_viptables_add (d, rule_hnd, position);
> + break;
> + }
> + case V4VOP_viptables_del:
> + {
> + uint32_t position = arg4;
> + XEN_GUEST_HANDLE (v4v_viptables_rule_t) rule_hnd =
> + guest_handle_cast (arg1, v4v_viptables_rule_t);
> + rc = -EPERM;
> + if (!IS_PRIV(d))
> + goto out;
> + rc = v4v_viptables_del (d, rule_hnd, position);
> + break;
> + }
> + case V4VOP_viptables_list:
> + {
> + XEN_GUEST_HANDLE (v4v_viptables_list_t) rules_list_hnd =
> + guest_handle_cast(arg1, v4v_viptables_list_t);
> + rc = -EPERM;
> + if (!IS_PRIV(d))
> + goto out;
> + rc = v4v_viptables_list (d, rules_list_hnd);
> + break;
> + }
> default:
> rc = -ENOSYS;
> break;
> diff --git a/xen/include/public/v4v.h b/xen/include/public/v4v.h
> index 197770e..d8ca507 100644
> --- a/xen/include/public/v4v.h
> +++ b/xen/include/public/v4v.h
> @@ -213,6 +213,9 @@
> * NULL, NULL, nent, 0)
> */
>
> +#define V4VOP_viptables_add 6
> +#define V4VOP_viptables_del 7
> +#define V4VOP_viptables_list 8
This will need documentation, and descriptions of the arguments
(assuming the actual definitions don't end up moving back into this
file).
And they should probably go below the V4VOP_sendv definition.
Cheers,
Tim.
> #define V4VOP_sendv 5
> /*
> diff --git a/xen/include/xen/v4v.h b/xen/include/xen/v4v.h
> index 641a6a8..e5b4cb7 100644
> --- a/xen/include/xen/v4v.h
> +++ b/xen/include/xen/v4v.h
> @@ -103,6 +103,32 @@ struct v4v_ring_message_header
> uint8_t data[0];
> } V4V_PACKED;
>
> +typedef struct v4v_viptables_rule
> +{
> + struct v4v_addr src;
> + struct v4v_addr dst;
> + uint32_t accept;
> +} V4V_PACKED v4v_viptables_rule_t;
> +
> +DEFINE_XEN_GUEST_HANDLE (v4v_viptables_rule_t);
> +
> +struct v4v_viptables_rule_node
> +{
> + struct list_head list;
> + struct v4v_addr src;
> + struct v4v_addr dst;
> + uint32_t accept;
> +} V4V_PACKED;
> +
> +typedef struct v4v_viptables_list
> +{
> + uint32_t start_rule;
> + uint32_t nb_rules;
> + struct v4v_viptables_rule rules[0];
> +} V4V_PACKED v4v_viptables_list_t;
> +
> +DEFINE_XEN_GUEST_HANDLE (v4v_viptables_list_t);
> +
> /*
> * Helper functions
> */
_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel
|
0005-v4v-Introduce-basic-access-control-to-V4V.patch
