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 10:05:28 +0100 Message-ID: <20140130090528.GI17001@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-la0-f51.google.com (mail-la0-f51.google.com [209.85.215.51]) by gabe.freedesktop.org (Postfix) with ESMTP id F0A93FC8F7 for ; Thu, 30 Jan 2014 01:05:35 -0800 (PST) Received: by mail-la0-f51.google.com with SMTP id c6so2281106lan.24 for ; Thu, 30 Jan 2014 01:05:33 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140130085328.GG17001@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 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch