Re: [PATCH 02/10] drm/radeon: UVD bringup v7

[Jerome Glisse <j.glisse@gmail.com>]

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