Re: [Xen-devel] [PATCH v5 06/28] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> +static struct payload *find_payload(const xen_xsplice_name_t *name)
> +{
> +    struct payload *data, *found = NULL;
> +    char n[XEN_XSPLICE_NAME_SIZE];
> +    int rc;
> +
> +    rc = verify_name(name);
> +    if ( rc )
> +        return ERR_PTR(rc);
> +
> +    if ( __copy_from_guest(n, name->name, name->size) )
> +        return ERR_PTR(-EFAULT);
> +
> +    if ( n[name->size - 1] )
> +        return ERR_PTR(-EINVAL);
> +
> +    spin_lock_recursive(&payload_lock);
> +
> +    list_for_each_entry ( data, &payload_list, list )
> +    {
> +        if ( !strcmp(data->name, n) )
> +        {
> +            found = data;
> +            break;
> +        }
> +    }
> +
> +    spin_unlock_recursive(&payload_lock);
> +
> +    return found;
> +}

So I've mentioned this before: Dropping the lock here means the
caller may (in general) not de-reference the returned pointer. While
it's only xsplice_upload() that calls this with the lock not held, and
that function indeed doesn't de-ref the pointer, it's still racy: Two
xsplice_upload() invocations for identically named payloads would
then possibly insert both instead of one of them detecting the
collision reliably. Hence I think all callers need to hold the lock, and
hence the need for a recursive lock goes away.

> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> +    struct payload *data;
> +    int rc;
> +
> +    rc = verify_payload(upload);
> +    if ( rc )
> +        return rc;
> +
> +    data = find_payload(&upload->name);
> +    if ( data && !IS_ERR(data) /* Found. */ )
> +        return -EEXIST;
> +
> +    if ( IS_ERR(data) )
> +        return PTR_ERR(data);
> +
> +    data = xzalloc(struct payload);
> +    if ( !data )
> +        return -ENOMEM;
> +
> +    rc = -EFAULT;
> +    if ( __copy_from_guest(data->name, upload->name.name, upload->name.size) 
> )
> +        goto out;

You're reading the string from guest memory a second time here,
invalidating the verification done earlier.

> +static int xsplice_list(xen_sysctl_xsplice_list_t *list)
> +{
> +    xen_xsplice_status_t status;
> +    struct payload *data;
> +    unsigned int idx = 0, i = 0;
> +    int rc = 0;
> +
> +    if ( list->nr > 1024 )
> +        return -E2BIG;
> +
> +    if ( list->pad )
> +        return -EINVAL;
> +
> +    if ( list->nr &&
> +         (!guest_handle_okay(list->status, list->nr) ||
> +          !guest_handle_okay(list->name, XEN_XSPLICE_NAME_SIZE * list->nr) ||
> +          !guest_handle_okay(list->len, list->nr)) )
> +        return -EINVAL;
> +
> +    spin_lock_recursive(&payload_lock);
> +    if ( list->idx > payload_cnt )

>= ?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel