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, > > > > > > > [email protected] 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 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
