<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 31, 2016 at 5:13 PM, Matthew Auld <span dir="ltr"><<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On 31 October 2016 at 16:27, Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> wrote:<br>
><br>
><br>
> On Fri, Oct 28, 2016 at 3:27 PM, Matthew Auld<br>
> <<a href="mailto:matthew.william.auld@gmail.com">matthew.william.auld@gmail.<wbr>com</a>> wrote:<br>
>><br>
>> > +/* Note we copy the properties from userspace outside of the i915 perf<br>
>> > + * mutex to avoid an awkward lockdep with mmap_sem.<br>
>> > + *<br>
>> > + * Note this function only validates properties in isolation it doesn't<br>
>> > + * validate that the combination of properties makes sense or that all<br>
>> > + * properties necessary for a particular kind of stream have been set.<br>
>> > + */<br>
>> > +static int read_properties_unlocked(<wbr>struct drm_i915_private *dev_priv,<br>
>> > +                                   u64 __user *uprops,<br>
>> > +                                   u32 n_props,<br>
>> > +                                   struct perf_open_properties *props)<br>
>> > +{<br>
>> > +       u64 __user *uprop = uprops;<br>
>> > +       int i;<br>
>> > +<br>
>> > +       memset(props, 0, sizeof(struct perf_open_properties));<br>
>> > +<br>
>> > +       if (!n_props) {<br>
>> > +               DRM_ERROR("No i915 perf properties given");<br>
>> > +               return -EINVAL;<br>
>> > +       }<br>
>> > +<br>
>> > +       if (n_props > DRM_I915_PERF_PROP_MAX) {<br>
>> Ah but DRM_I915_PERF_PROP_MAX is not a property itself.<br>
><br>
><br>
> I'm not sure I follow what your implied concern is?<br>
><br>
> This is just a sanity check for the number properties given by userspace,<br>
> based on the assumption that there's currently no reason for multiple values<br>
> with a particular property id.<br>
><br>
</div></div>All I meant was should it not be n_props >= DRM_I915_PERF_PROP_MAX ? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
So with that fixed, or if I'm completely mad:<br>
Reviewed-by: Matthew Auld <<a href="mailto:matthew.auld@intel.com">matthew.auld@intel.com</a>><br></blockquote><div><br></div><div>Ah, I see. Actually tbh I think either is reasonable...<br><br></div><div>The check is mainly about ruling out the silly large values that could be given, imposing a upper-bound to the number of properties expected from userspace. It might help catch userspace giving garbage/undefined data, or block attempts to get the kernel parsing huge amounts of property data which should never be necessary for configuring a stream. It doesn't e.g. stop userspace specifying duplicate property IDs even if they supply less than the maximum allowed. So even if it allowed say 2x the number of properties I think it would still pretty much do its job.<br><br></div><div>I could imagine in the future the same check might become much more fuzzy if we have a case where userspace might need to legitimately specify the same property ID multiple times (where the sequential order is relevant).<br></div><div><br></div><div>_PERF_PROP_MAX is the last in the enum whereby we can interpret it as an upper bound on the number of properties while we don't currently expect to see property IDs duplicated.<br><br></div><div>The detail here though is that ID 0 is reserved  so _PERF_PROP_MAX is more like ('the maximum number of properties' + 1) - and so this is what you're essentially highlighting.<br></div><div><br></div><div>I can change this - maybe with a comment about ID 0 being reserved and explaining the assumption that property ID duplicates aren't currently expected<br></div></div><br>Thanks for the review!<br><br></div></div>