linux-kernel - Re: [PATCH 08/18] xen/pvcalls: implement connect command

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9515ef24-ccad-f148-bbe3-824844d01223@oracle.com>
Date:   Tue, 16 May 2017 17:56:43 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Stefano Stabellini <sstabellini@...nel.org>
Cc:     xen-devel@...ts.xen.org, linux-kernel@...r.kernel.org,
        jgross@...e.com, Stefano Stabellini <stefano@...reto.com>
Subject: Re: [PATCH 08/18] xen/pvcalls: implement connect command


>>> +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
>>> +	if (ret < 0) {
>>> +		sock_release(map->sock);
>>> +		kfree(map);
>>> +		goto out;
>>> +	}
>>> +	map->ring = page;
>>> +	map->ring_order = map->ring->ring_order;
>>> +	/* first read the order, then map the data ring */
>>> +	virt_rmb();
>>
>> Not sure I understand what the barrier is for here. I don't think compiler
>> will reorder ring_order access with the call.
> It's to avoid using the live version of ring_order to map the data ring
> pages (the other end could be changing that value at any time). We want
> to be sure that the compiler doesn't optimize out map->ring_order and
> use map->ring->ring_order instead.

Wouldn't WRITE_ONCE(map->ring_order, map->ring->ring_order) be the right
primitive then?

And also: if the other side changes ring size, what are we mapping then?
It's obsolete by now.

-boris

>
>
>>> +	if (map->ring_order > MAX_RING_ORDER) {
>>> +		ret = -EFAULT;
>>> +		goto out;
>>> +	}
>> If the barrier is indeed needed this check belongs before it.
> I don't think so, see above.
>
>
>>
>>> +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
>>> +				     (1 << map->ring_order), &page);
>>> +	if (ret < 0) {
>>> +		sock_release(map->sock);
>>> +		xenbus_unmap_ring_vfree(dev, map->ring);
>>> +		kfree(map);
>>> +		goto out;
>>> +	}
>>> +	map->bytes = page;
>>>