[ovs-dev] [PATCH 2/3] datapath: Prepare to support 32-bit compatibility ioctls.

[Jesse Gross]

On Mon, May 10, 2010 at 2:13 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> +static int do_answer_query(struct sw_flow *flow, u32 query_flags,
> +                          struct odp_flow_stats __user *ustats,
> +                          union odp_action __user *actions,
> +                          u32 __user *n_actionsp)
>  {
> -       union odp_action __user *actions;
>        struct sw_flow_actions *sf_acts;
> +       struct odp_flow_stats stats;
> +       unsigned long int flags;
>        u32 n_actions;
>
> -       if (__get_user(actions, &ufp->actions) ||
> -           __get_user(n_actions, &ufp->n_actions))
> +       spin_lock_irqsave(&flow->lock, flags);
> +       get_stats(flow, &stats);
> +       if (query_flags & ODPFF_ZERO_TCP_FLAGS) {
> +               flow->tcp_flags = 0;
> +       }
> +       spin_unlock_irqrestore(&flow->lock, flags);
> +
> +       if (__copy_to_user(ustats, &stats, sizeof(struct odp_flow_stats))
> ||
> +           __get_user(n_actions, n_actionsp))
>                return -EFAULT;
>
>        if (!n_actions)
>                return 0;
>
>        sf_acts = rcu_dereference(flow->sf_acts);
> -       if (__put_user(sf_acts->n_actions, &ufp->n_actions) ||
> +       if (__put_user(sf_acts->n_actions, n_actionsp) ||
>            (actions && copy_to_user(actions, sf_acts->actions,
>                                     sizeof(union odp_action) *
>                                     min(sf_acts->n_actions, n_actions))))
>

All these __[copy|get|put] functions make me nervous because we need to
ensure that the caller has already called access_ok().  Besides requiring
thought, I'm pretty sure that there are places where we do a write to
userspace without calling access_ok(VERIFY_WRITE, ...) - just a read check.

I know these existed before (and there are a few other of them scattered
around) but unless it is obviously correct like access_ok() followed by a
series of reads/writes it just doesn't seem worth avoiding the check for a
very marginal performance benefit.


> -static int list_flows(struct datapath *dp, const struct odp_flowvec
> *flowvec)
> +static int do_list_flows(struct datapath *dp, const struct odp_flowvec
> *flowvec)
>  {
>        struct list_flows_cbdata cbdata;
>        int error;
> @@ -1218,7 +1239,8 @@ static int do_flowvec_ioctl(struct datapath *dp,
> unsigned long argp,
>            copy_from_user(&flowvec, uflowvec, sizeof flowvec))
>                return -EFAULT;
>
> -       if (flowvec.n_flows > INT_MAX / sizeof(struct odp_flow))
> +       if (flowvec.n_flows < 0 ||
> +           flowvec.n_flows > INT_MAX / sizeof(struct odp_flow))
>                return -EINVAL;
>

I don't think that we need this check to see if 'n_flows' is negative now
that it is unsigned.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100512/69757c2f/attachment-0003.html>