From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 02/13] drm/i915: Implement command buffer parsing logic Date: Thu, 30 Jan 2014 12:07:15 +0100 Message-ID: <20140130110715.GO17001@phenom.ffwll.local> References: <1385484699-51596-1-git-send-email-bradley.d.volkin@intel.com> <1391032514-19136-1-git-send-email-bradley.d.volkin@intel.com> <1391032514-19136-3-git-send-email-bradley.d.volkin@intel.com> <20140129222836.GN28110@nuc-i3427.alporthouse.com> <20140130085328.GG17001@phenom.ffwll.local> <20140130090528.GI17001@phenom.ffwll.local> <20140130091206.GK17001@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C47C8A225 for ; Thu, 30 Jan 2014 03:07:21 -0800 (PST) Received: by mail-lb0-f182.google.com with SMTP id w7so2426739lbi.13 for ; Thu, 30 Jan 2014 03:07:19 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140130091206.GK17001@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Chris Wilson , bradley.d.volkin@intel.com, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Jan 30, 2014 at 10:12:06AM +0100, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 10:05:28AM +0100, Daniel Vetter wrote: > > On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote: > > > On Wed, Jan 29, 2014 at 10:28:36PM +0000, Chris Wilson wrote: > > > > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.volkin@intel.com wrote: > > > > > +/* > > > > > + * Returns a pointer to a descriptor for the command specified by cmd_header. > > > > > + * > > > > > + * The caller must supply space for a default descriptor via the default_desc > > > > > + * parameter. If no descriptor for the specified command exists in the ring's > > > > > + * command parser tables, this function fills in default_desc based on the > > > > > + * ring's default length encoding and returns default_desc. > > > > > + */ > > > > > +static const struct drm_i915_cmd_descriptor* > > > > > +find_cmd(struct intel_ring_buffer *ring, > > > > > + u32 cmd_header, > > > > > + struct drm_i915_cmd_descriptor *default_desc) > > > > > +{ > > > > > + u32 mask; > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < ring->cmd_table_count; i++) { > > > > > + const struct drm_i915_cmd_descriptor *desc; > > > > > + > > > > > + desc = find_cmd_in_table(&ring->cmd_tables[i], cmd_header); > > > > > + if (desc) > > > > > + return desc; > > > > > + } > > > > > + > > > > > + mask = ring->get_cmd_length_mask(cmd_header); > > > > > + if (!mask) > > > > > + return NULL; > > > > > + > > > > > + BUG_ON(!default_desc); > > > > > + default_desc->flags = CMD_DESC_SKIP; > > > > > + default_desc->length.mask = mask; > > > > > > > > If we turn off all hw validation (through use of the secure bit) should > > > > we not default to a whitelist of commands? Otherwise it just seems to be > > > > a case of running a fuzzer until we kill the machine. > > > > > > Preventing hangs and dos is imo not the attack model, gpus are too fickle > > > for that. The attach model here is to prevent priveledge escalation and > > > information leaks. I.e. we want just containement of all read/write access > > > to the gtt space. > > > > > > I think for that purpose an explicit whitelist of commands which target > > > things outside of the (pp)gtt is sufficient. radeon's checker design is > > > completely different, but pretty much the only command they have is > > > to load register values. Intel gpus otoh have a big set of special-purpose > > > commands to load (most) of the rendering pipeline state. So we have > > > hw built-in register whitelists for all that stuff since you just can't > > > load arbitrary registers and state with those commands. > > > > > > Also note that for raw register access Bradley's scanner _is_ whitelist > > > based. And for general reads/writes gpu designers confirmed that those are > > > all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so > > > as long as we check for the exceptions and otherwise only whitelist MI_ > > > commands we know about we should be covered. > > > > > > So I think this is sound. > > > > Hm, but while scrolling through the checker I haven't spotted a "reject > > everything unknown" for MI_CLIENT commands. Bradley, have I missed that? > > > > I think submitting an invented MI_CLIENT command would also be a good > > testcase. > > One more: I think it would be good to have an overview comment at the top > of i915_cmd_parser.c which details the security attack model and the > overall blacklist/whitelist design of the checker. We don't (yet) have > autogenerated documentation for i915, but that's something I'm working on. > And the kerneldoc system can also pull in multi-paragraph overview > comments besides the usual api documentation, so it's good to have things > ready. Chatted with Chris a bit more on irc about this, and for more paranoia I guess we should also reject any unknown client and media subclient commands. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch