[Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic

[Daniel Vetter]

On Mon, Feb 03, 2014 at 03:00:19PM -0800, Volkin, Bradley D wrote:
> Ping. Daniel or Chris, can one of you clarify this request? Thanks.

I've been enjoying fosdem ...

> On Thu, Jan 30, 2014 at 10:05:27AM -0800, Volkin, Bradley D wrote:
> > On Thu, Jan 30, 2014 at 03:07:15AM -0800, Daniel Vetter wrote:
> > > 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 at 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.
> > 
> > Hmm, not sure I follow. Can you elaborate?
> > 
> > Are you suggesting we add all the MI and Media commands to the tables and reject
> > any command from those client/subclients that is not found in the table? Or that
> > we look at the client and subclient fields of the command and reject if they are
> > not from a set of expected values? Or other?

Yeah, I think we should check the client/subclient fields for know values,
but not have explicit lists for each command. So overall control-flow
would be:

1. Check client/subclient against whitelist (per-ring obviously, so reject
blitter commands on non-blt rings ofc).

2. If the client indicates an MI command, check against MI_ whitelist.

3. For all other clients check against blacklist/greylist (e.g.
pipe_control where we just need to forbid global gtt writes).

Iirc we need a special-cases list for blitter commands anyway due to their
irregular lenght, so maybe we could do a full whitelist for that one, too.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch