On Tue, Dec 29, 2015 at 04:49:01PM +0100, Michał Winiarski wrote:
> @@ -994,6 +1013,20 @@ validate_exec_list(struct drm_device *dev,
> if (exec[i].flags & invalid_flags)
> return -EINVAL;
>
> + /* Offset can be used as input (EXEC_OBJECT_PINNED), reject
> + * any non-page-aligned or non-canonial addresses.
> + */
> + if (exec[i].flags & EXEC_OBJECT_PINNED &&
> + (exec[i].offset != gen8_canonical_addr(exec[i].offset) ||
> + offset_in_page(exec[i].offset)))
> + return -EINVAL;
> +
if (exec[i].flags & EXEC_OBJECT_PINNED) {
if (exec[i].offset != gen8_canonical_addr(exec[i].offset & PAGE_MASK))
return -EINVAL;
/* From drm_mm perspective address space is continuous,
* so from this point we're always using non-canonical form
* internally.
*/
exec[i].offset &= (1ULL << 48) - 1;
}
Splitting up the two tests just makes it a bit easier to read (imo, and
I've been told on numerous occasions to do the same :) Whilst not as
obvious atm, it also helps when we have multiple extension checks in the
validate(). As a secondary point, we can then also demonstate that we
can fully restrict manipulating exec[i].offset to the pinned path.
#define GEN8_HIGH_ADDRESS_BIT 47
#define GEN8_ADDRESS_MASK (1ULL << (GEN8_HIGH_ADDRESS_BIT+1)) - 1
GEN8_CANONICAL_HIGH_BIT ?
since we have two places now that know about the address format, or
perhaps
static u64 gen8_undo_canonical_addr(u64);
exec[i].offset = gen8_undo_canonical_addr(exec[i].offset);
so that we can put them next to each other. That seems a better idea.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
