Re: [PATCH 02/13] drm/i915: Implement command buffer parsing logic

[Daniel Vetter <daniel@ffwll.ch>]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch