|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver
On 04/25/2018 08:16 PM, Dongwon Kim wrote: On Wed, Apr 25, 2018 at 08:34:55AM +0200, Daniel Vetter wrote:On Wed, Apr 25, 2018 at 09:07:07AM +0300, Oleksandr Andrushchenko wrote:On 04/24/2018 11:35 PM, Dongwon Kim wrote:Had a meeting with Daniel and talked about bringing out generic part of hyper-dmabuf to the userspace, which means we most likely reuse IOCTLs defined in xen-zcopy for our use-case if we follow his suggestion.I will still have kernel side API, so backends/frontends implemented in the kernel can access that functionality as well.So assuming we use these IOCTLs as they are, Several things I would like you to double-check.. 1. returning gref as is to the user space is still unsafe because it is a constant, easy to guess and any process that hijacks it can easily exploit the buffer. So I am wondering if it's possible to keep dmabuf-to -gref or gref-to-dmabuf in kernel space and add other layers on top of those in actual IOCTLs to add some safety.. We introduced flink like hyper_dmabuf_id including random number but many says even that is still not safe.Yes, it is generally unsafe. But even if we have implemented the approach you have in hyper-dmabuf or similar, what stops malicious software from doing the same with the existing gntdev UAPI? No need to brute force new UAPI if there is a simpler one. That being said, I'll put security aside at the first stage, but of course we can start investigating ways to improve (I assume you already have use-cases where security issues must be considered, so, probably you can tell more on what was investigated so far).Yeah, although we think we lowered the chance of guessing the right id by adding random number to it, the security hole is still there as far as we use a constant id across VMs. We understood this from the beginning but couldn't find a better way. So what we proposed is to make sure our customer understand this and prepare very secure way to handle this id in the userspace (mattrope however recently proposed a "hyper-pipe" which FD-type id can be converted and exchanged safely through. So we are looking into this now.) And another approach we have proposed is to use event-polling, that lets the privileged userapp in importing guest to know about a new exported DMABUF so that it can retrieve it from the queue then redistribute to other applications. This method is not very flexible however, is one way to hide ID from userspace completely. Anyway, yes, we can continue to investigate the possible way to make it more secure. Great, if you come up with something then you'll be able to plumb this in Maybe a bit more context here: So in graphics we have this old flink approach for buffer sharing with processes, and it's unsafe because way too easy to guess the buffer handles. And anyone with access to the graphics driver can then import that buffer object. We switched to file descriptor passing to make sure only the intended recipient can import a buffer. So at the vm->vm level it sounds like grefs are safe, because they're only for a specific other guest (or sets of guests, not sure about). That means security is only within the OS. For that you need to make sure that unpriviledge userspace simply can't ever access a gref. If that doesn't work out, then I guess we should improve the xen gref stuff to have a more secure cookie.2. maybe we could take hypervisor-independent process (e.g. SGT<->page) out of xen-zcopy and put those in a new helper library.I believe this can be done, but at the first stage I would go without that helper library, so it is clearly seen what can be moved to it later (I know that you want to run ACRN as well, but can I run it on ARM? ;)There's already helpers for walking sgtables and adding pages/enumerating pages. I don't think we need more.ok, where would that helpers be located? If we consider we will use these with other hypervisor drivers, maybe it's better to place those in some common area? I am not quite sure what and if those helpers be really needed. Let's try to prototype the thing and then see what can be moved to a helper library and where it should live 3. please consider the case where original DMA-BUF's first offset and last length are not 0 and PAGE_SIZE respectively. I assume current xen-zcopy only supports page-aligned buffer with PAGE_SIZE x n big.Hm, what is the use-case for that?Just in general use-case.. I was just considering the case (might be corner case..) where sg->offset != 0 or sg->length != PAGE_SIZE. Hyper dmabuf sends this information (first offset and last length) together with references for pages. So I was wondering if we should so similar thing in zcopy since your goal is now to cover general dma-buf use-cases (however, danvet mentioned hard constaint of dma-buf below.. so if this can't happen according to the spec, then we can ignore it..) I won't be considering this use-case during prototyping as it seems it doesn't have a *real* ground underneath dma-buf is always page-aligned. That's a hard constraint of the linux dma-buf interface spec. -DanielHmm.. I am little bit confused.. So does it mean dmabuf->size is always n*PAGE_SIZE? What is the sgt behind dmabuf has an offset other than 0 for the first sgl or the length of the last sgl is not PAGE_SIZE? You are saying this case is not acceptable for dmabuf? IMO, yes, see above thanks, DWThank you, OleksandrOn Tue, Apr 24, 2018 at 02:59:39PM +0300, Oleksandr Andrushchenko wrote:On 04/24/2018 02:54 PM, Daniel Vetter wrote:On Mon, Apr 23, 2018 at 03:10:35PM +0300, Oleksandr Andrushchenko wrote:On 04/23/2018 02:52 PM, Wei Liu wrote:On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko wrote: _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |