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.vol...@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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx