From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 02/10] drm/radeon: UVD bringup v7 Date: Thu, 04 Apr 2013 15:26:02 +0200 Message-ID: <515D7F6A.306@vodafone.de> References: <1364944719-5175-1-git-send-email-deathsimple@vodafone.de> <1364944719-5175-3-git-send-email-deathsimple@vodafone.de> <20130403145323.GC2010@gmail.com> <515C5093.9030709@vodafone.de> <20130403171053.GB3244@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 7FFEFE5CE8 for ; Thu, 4 Apr 2013 06:26:07 -0700 (PDT) In-Reply-To: <20130403171053.GB3244@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Am 03.04.2013 19:10, schrieb Jerome Glisse: > On Wed, Apr 03, 2013 at 05:53:55PM +0200, Christian K=F6nig wrote: >> Am 03.04.2013 16:53, schrieb Jerome Glisse: >>> On Wed, Apr 03, 2013 at 01:18:31AM +0200, Christian K=F6nig 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 =3D &p->chunks[p->chunk_relocs_idx]; >>>> + idx =3D radeon_get_ib_value(p, data1); >>>> + if (idx >=3D relocs_chunk->length_dw) { >>>> + DRM_ERROR("Relocs at %d after relocations chunk end %d !\n", >>>> + idx, relocs_chunk->length_dw); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + reloc =3D p->relocs_ptr[(idx / 4)]; >>>> + start =3D reloc->lobj.gpu_offset; >>>> + end =3D start + radeon_bo_size(reloc->robj); >>>> + start +=3D radeon_get_ib_value(p, data0); >>> I am assuming there is no way for you to know the size that the uvd eng= ine 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 ca= se. > 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 V= RAM. > > I have big doubt on that given the 256M window, i fear that it might go b= ack > to writting to begining of memory where the page table is. Crafting an attack from it would still be a bit tricky because it is = compressed image data that gets written, but never less it is indeed = possible. > 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. Well not exactly, it is planned that the 256M limit goes away with some = of the next hw generations. And at least at this point we need to make = sure that UVD never writes somewhere it shouldn't. Anyway moving the = page table to not CPU accessible VRAM sounds like a pretty good idea. > If we can have such assurance i guess we can make uvd as an option and ma= ke > a very explicit comment stating that UVD engine can be use as an exploit > vector path. I think I will just sit down and implement size checking, at least for = the destination buffer, cause after all that's just a texture. And for = the reference buffer I maybe just use what userspace send to the = hardware as buffer size, and make a sanity check on that one also. Ok, need to think about it a bit more. > Jerome >