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

["Volkin, Bradley D" <bradley.d.volkin@intel.com>]

On Tue, Feb 04, 2014 at 02:20:36AM -0800, Daniel Vetter wrote:
> 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@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).

Ok, that's easy enough. I'm not sure the benefit is that large, but I can add this.

> 
> 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).

I'm a bit concerned about 2 and 3 because the behavior and table structures seem
fairly different from what we have now. I'm hesitant to make too big a change here
so close to having something functional.

Please correct me if I've missed or misunderstood anything, but my thinking is...

The current table structure is that we have tables per-ring and per-gen (plus the table
for common MI commands) and all tables are treated as blacklist/greylist. The proposed
flow here would indicate that we need tables per-ring, per-client, per-gen and that some
would be treated as a whitelist and some as a blacklist/greylist.

I think the benefit to these changes amounts to preventing clients from issuing invalid
MI commands, but clients can do that today, so it's not a regression right? It could also
make it easier to safely cover new platforms if MI commands were added, though the parser
is strictly limited to gen7 today and would need additional work to enable it for new gens.

The set of MI commands for current gens is small compared to the other command ranges.
On the one hand, that makes it easier to create a whitelist of the commands. On the other
hand, it also makes it easy to just audit the commands in the spec and validate that the
blacklist/greylist covers the potential issues.

So, if we maintain the conservative parser enabling and re-audit the lists when enabling
new gens, and if we can live with clients issuing totally invalid commands (and I get the
feeling that we have to), then I think the current solution gets us the benefits we want
with less complexity.

Let me know what you think. I'll continue working through the other feedback either way.

> 
> 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.

All rings have commands with irregular length encodings. I believe they also all have
commands with non-fixed lengths (e.g. XY_TEXT_IMMEDIATE_BLT on blt, MEDIA_OBJECT on render).

Thanks,
Brad

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