On Wed, Apr 03, 2013 at 05:53:55PM +0200, Christian König wrote: > Am 03.04.2013 16:53, schrieb Jerome Glisse: > >On Wed, Apr 03, 2013 at 01:18:31AM +0200, Christian König wrote: > >>[SNIP] > >> > >> /* hardcode those limit for now */ > >> #define RADEON_VA_IB_OFFSET (1 << 20) > >> #define RADEON_VA_RESERVED_SIZE (8 << 20) > >>@@ -357,8 +360,9 @@ struct radeon_bo_list { > >> struct ttm_validate_buffer tv; > >> struct radeon_bo *bo; > >> uint64_t gpu_offset; > >>- unsigned rdomain; > >>- unsigned wdomain; > >>+ bool written; > >>+ unsigned domain; > >>+ unsigned alt_domain; > >> u32 tiling_flags; > >> }; > >I think that the change to the rdomain/wdomain should be in a patch > >of its own. I think the change is fine but we had issue with change > >that touched that part previously, would make bisecting and > >understanding the change implication easier. > > Agree, I actually planed to do so, but for the whole IP review stuff > we needed to maintain a more or less stable patch base. Long story, > but I'm going to change it. > > >>@@ -826,7 +830,6 @@ struct radeon_cs_reloc { > >> struct radeon_bo *robj; > >> struct radeon_bo_list lobj; > >> uint32_t handle; > >>- uint32_t flags; > >> }; > >Why removing the flags ? iirc it's not really use right now but i > >remember plan to use it. > > Ups, just a rebasing artifact. But when it's unused we should remove > it, probably just not in this patch. > > >>[SNIP] > >> > >>+static int radeon_uvd_cs_reloc(struct radeon_cs_parser *p, int data0, int data1) > >>+{ > >>+ struct radeon_cs_chunk *relocs_chunk; > >>+ struct radeon_cs_reloc *reloc; > >>+ unsigned idx, cmd; > >>+ uint64_t start, end; > >>+ > >>+ relocs_chunk = &p->chunks[p->chunk_relocs_idx]; > >>+ idx = radeon_get_ib_value(p, data1); > >>+ if (idx >= relocs_chunk->length_dw) { > >>+ DRM_ERROR("Relocs at %d after relocations chunk end %d !\n", > >>+ idx, relocs_chunk->length_dw); > >>+ return -EINVAL; > >>+ } > >>+ > >>+ reloc = p->relocs_ptr[(idx / 4)]; > >>+ start = reloc->lobj.gpu_offset; > >>+ end = start + radeon_bo_size(reloc->robj); > >>+ start += radeon_get_ib_value(p, data0); > >I am assuming there is no way for you to know the size that the uvd engine will write to ? > >You are not checking anything on uvd possibly overwritting after the bo end. > > Yeah that gave me headache for a quite long time, too. The problem > is to figure out how much is actually written you need to keep track > of the whole lot of informations including the UVD session, > create/decode/destroy messages and allot of fiddling with the codec > specific parameters. > > And if I understand the UVD internals correctly even if we check > everything there is no guarantee that a special crafted bitstream > could not let UVD to write over the end of the buffer.... > > Is it ok if we but a big TODO on it for the initial patch? > > Cheers, > Christian. I think i only need one assurance and i think for uvd this will be the case. If UVD block write past bo end can you be sure that no matter what it will overwritte to address > start ie it could not overwritte to begining of VRAM. I have big doubt on that given the 256M window, i fear that it might go back to writting to begining of memory where the page table is. Note that i think that now that we have cp dma pagetable entry update we can probably just move the pagetable to end of vram on 90% GPU with UVD this will be > 256M which seems like a zone where UVD can never write. If we can have such assurance i guess we can make uvd as an option and make a very explicit comment stating that UVD engine can be use as an exploit vector path. Cheers, Jerome _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel