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
