[PATCH 0/2] drm/imagination: improve UAPI heap and static areas query ioctls

[PATCH 0/2] drm/imagination: improve UAPI heap and static areas query ioctls

[Luigi Santivetti]

This series is about better protecting userspace when either one of the
heap or the static data areas query IOCTL is made as we go forward and
extend the UAPI introducing new heaps or new fields to their related
data types.

Please refer to each patch commit message which describes in some detail
the nature of the issue and the solution for it.

In general terms this series makes the UAPI more flexible, introducing a
distinction between a legacy (or minimum) and a current (or maximum) value
for both the stride and the count that is returned to userspace.

With the new pattern in place, an older[*] version of userspace defaults
to requesting and to obtaining the legacy (or minimum) value of both stride
and count and this makes safe to update their maximum value going forward.

[*] before https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/44456

Signed-off-by: Luigi Santivetti <luigi.santivetti@imgtec.com>
---
Luigi Santivetti (2):
      drm/imagination: clamp stride of heaps and static areas for legacy UM
      drm/imagination: clamp count of heaps and static areas for legacy UM

 drivers/gpu/drm/imagination/pvr_drv.h | 33 ++++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_vm.c  | 32 ++++++++++++++++++---
 include/uapi/drm/pvr_drm.h            | 53 +++++++++++++++++++++++++++++++----
 3 files changed, 108 insertions(+), 10 deletions(-)
---
base-commit: 33ed26235db93162935d1719bc6f538c384a2fd8
change-id: 20261104-ioctl-query-count-stride-fixes-43b4df2c2fc8

Best regards,
--  
Luigi Santivetti <luigi.santivetti@imgtec.com>

[PATCH 1/2] drm/imagination: clamp stride of heaps and static areas for legacy UM

[Luigi Santivetti]

Userspace before 4c18bbc6 in [1] failed to set the stride when doing the
DRM_PVR_DEV_QUERY_HEAP_INFO_GET and DRM_PVR_DEV_QUERY_STATIC_DATA_AREAS_GET
query ioctl calls. This isn't a problem as long as the kernel and userspace
use the same version of struct drm_pvr_heap and of drm_pvr_static_data_area
and at present this is the case. However, nothing prevents it to change in
the future, allowing userspace and kernel to run with a different version
of either of those structures.

In order to protect userspace before 4c18bbc6, change those ioctl calls now
future proofing the value of the stride that is returned by the kernel.
This means to take a snapshot of the size of struct drm_pvr_heap and of
drm_pvr_static_data_area as of today and return it to old UMDs. Since the
size has never changed so far, we may refer to this size in docs, code and
comments as version 1, first released version, legacy or minimum supported
version of the stride.

In order to be safe for userspace, make use of the following properties
that hold in UM code before 4c18bbc6:

  1. for both DRM_PVR_DEV_QUERY_{HEAP_INFO,STATIC_DATA_AREAS}_GET, it makes
     two ioctl calls
  2. it never sets struct drm_pvr_obj_array::stride in either ioctl calls
  3. it assumes the stride from the kernel to match the stride of userspace

In the second ioctl call the value of the stride is set to what the kernel
had set it to in the first. Therefore, special case what is returned to UM
in the first call, so that the value of the stride matches with the one
userspace uses before 4c18bbc6.

Although backwards compatible, this commit alters the semantics of how to
retrieve the non-legacy or maximum supported stride going forward. The new
pattern for interacting with the kernel is documented in the UAPI header.

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/44456

Signed-off-by: Luigi Santivetti <luigi.santivetti@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_drv.h | 21 +++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_vm.c  | 14 ++++++++++++--
 include/uapi/drm/pvr_drm.h            | 33 +++++++++++++++++++++++++++------
 3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
index 7fa147312dd1..b9e0146c90b1 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.h
+++ b/drivers/gpu/drm/imagination/pvr_drv.h
@@ -70,6 +70,27 @@ int pvr_set_uobj_array(const struct drm_pvr_obj_array *out, u32 min_stride, u32
 	PVR_UOBJ_DECL(struct drm_pvr_dev_query_static_data_areas, static_data_areas) \
 	)
 
+/*
+ * Minimum supported values that are fetched by DRM_PVR_DEV_QUERY_HEAP_INFO_GET
+ * and DRM_PVR_DEV_QUERY_STATIC_DATA_AREAS_GET ioctl calls from userspace.
+ *
+ * These values may depend on the GPU hardware and they must not change for
+ * cores that are already supported. However, new cores (and architectures)
+ * may have different minimum values and they should be introduced
+ * respecting the semantics described in the UAPI for Rogue.
+ */
+
+/* Size of the first released version of struct drm_pvr_heap for Rogue cores. */
+#define DRM_PVR_HEAP_STRIDE_MIN_SUPPORTED \
+	PVR_UOBJ_MIN_SIZE_INTERNAL(struct drm_pvr_heap, page_size_log2)
+
+/*
+ * Size of the first released version of struct drm_pvr_static_data_area for
+ * Rogue cores.
+ */
+#define DRM_PVR_STATIC_DATA_AREA_STRIDE_MIN_SUPPORTED \
+	PVR_UOBJ_MIN_SIZE_INTERNAL(struct drm_pvr_static_data_area, offset)
+
 /**
  * PVR_UOBJ_GET() - Copies from _src_usr_ptr to &_dest_obj.
  * @_dest_obj: The destination container object in kernel space.
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 304d0743c5aa..a369d73c6969 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -984,8 +984,14 @@ pvr_static_data_areas_get(const struct pvr_device *pvr_dev,
 		return err;
 
 	if (!query.static_data_areas.array) {
+		u32 *stride = &query.static_data_areas.stride;
+
+		if (!query.static_data_areas.stride)
+			*stride = DRM_PVR_STATIC_DATA_AREA_STRIDE_MIN_SUPPORTED;
+		else
+			*stride = sizeof(struct drm_pvr_static_data_area);
+
 		query.static_data_areas.count = ARRAY_SIZE(static_data_areas);
-		query.static_data_areas.stride = sizeof(struct drm_pvr_static_data_area);
 		goto copy_out;
 	}
 
@@ -1024,8 +1030,12 @@ pvr_heap_info_get(const struct pvr_device *pvr_dev,
 		return err;
 
 	if (!query.heaps.array) {
+		if (!query.heaps.stride)
+			query.heaps.stride = DRM_PVR_HEAP_STRIDE_MIN_SUPPORTED;
+		else
+			query.heaps.stride = sizeof(struct drm_pvr_heap);
+
 		query.heaps.count = ARRAY_SIZE(pvr_heaps);
-		query.heaps.stride = sizeof(struct drm_pvr_heap);
 		goto copy_out;
 	}
 
diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
index ccf6c2112468..fd7cf9e230c3 100644
--- a/include/uapi/drm/pvr_drm.h
+++ b/include/uapi/drm/pvr_drm.h
@@ -319,9 +319,19 @@ struct drm_pvr_heap {
  */
 struct drm_pvr_dev_query_heap_info {
 	/**
-	 * @heaps: Array of &struct drm_pvr_heap. If pointer is NULL, the count
-	 * and stride will be updated with those known to the driver version, to
-	 * facilitate allocation by the caller.
+	 * @heaps: Array of &struct drm_pvr_heap. To facilitate allocation by
+	 * the caller:
+	 *
+	 * - if &heaps.array is NULL, the count will be updated with the count
+	 *   of heaps known to the kernel driver.
+	 *
+	 * - If &heaps.array is NULL and &heaps.stride is set by the caller, the
+	 *   stride will be updated to the size of &struct drm_pvr_heap known to
+	 *   the kernel driver.
+	 *
+	 * - If &heaps.array is NULL and &heaps.stride is zero, the stride will
+	 *   be updated with a value that is the minimum supported by the kernel
+	 *   and this may differ based on the GPU ID.
 	 */
 	struct drm_pvr_obj_array heaps;
 };
@@ -427,9 +437,20 @@ struct drm_pvr_static_data_area {
  */
 struct drm_pvr_dev_query_static_data_areas {
 	/**
-	 * @static_data_areas: Array of &struct drm_pvr_static_data_area. If
-	 * pointer is NULL, the count and stride will be updated with those
-	 * known to the driver version, to facilitate allocation by the caller.
+	 * @static_data_areas: Array of &struct drm_pvr_static_data_area. To
+	 * facilitate allocation by the caller:
+	 *
+	 * - if &static_data_areas.array is NULL, the count will be updated with
+	 *   the count of static areas known to the kernel driver.
+	 *
+	 * - If &static_data_areas.array is NULL and &static_data_areas.stride
+	 *   is set by the caller, the stride will be updated to the size of
+	 *   &struct drm_pvr_static_data_area known to the kernel driver.
+	 *
+	 * - If &static_data_areas.array is NULL and &static_data_areas.stride
+	 *   is zero, the stride will be updated with a value that is the
+	 *   minimum supported by the kernel and this may differ based on the
+	 *   GPU ID.
 	 */
 	struct drm_pvr_obj_array static_data_areas;
 };

-- 
2.43.0

[PATCH 2/2] drm/imagination: clamp count of heaps and static areas for legacy UM

[Luigi Santivetti]

Userspace mishandled the count of DRM PVR heaps returned by the kernel in
the DRM_PVR_DEV_QUERY_HEAP_INFO_GET ioctl call. In particular not taking
into account that the value of count returned by the kernel can be larger
than the count defined in userspace. This issue hasn't manifested itself
so far because no new DRM PVR heap has ever been introduced, however new
heaps are expected to be added in the future.

Userspace fixed this problem with 2a983a3a in [1]. For the kernel, it is
possible to be safe with respect to those versions of userspace predating
the fix and at the same time to preserve backwards compatibility.

After this change, future versions of userspace can keep using the ioctl in
the exact same way they do now and, in addition, they _can_ use the newly
introduced pattern for querying the minimum and the maximum value of
supported heaps by following the documentation that is available in the
UAPI header.

DRM_PVR_STATIC_DATA_AREA_COUNT_MIN_SUPPORTED was added in order to keep the
DRM_PVR_DEV_QUERY_HEAP_INFO_GET and DRM_PVR_DEV_QUERY_STATIC_DATA_AREAS_GET
ioctls consistent with each other, but there is no known mishandling in the
latter from userspace.

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/44456

Signed-off-by: Luigi Santivetti <luigi.santivetti@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_drv.h | 12 ++++++++++++
 drivers/gpu/drm/imagination/pvr_vm.c  | 18 ++++++++++++++++--
 include/uapi/drm/pvr_drm.h            | 28 ++++++++++++++++++++++++----
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
index b9e0146c90b1..200208fda0a3 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.h
+++ b/drivers/gpu/drm/imagination/pvr_drv.h
@@ -91,6 +91,18 @@ int pvr_set_uobj_array(const struct drm_pvr_obj_array *out, u32 min_stride, u32
 #define DRM_PVR_STATIC_DATA_AREA_STRIDE_MIN_SUPPORTED \
 	PVR_UOBJ_MIN_SIZE_INTERNAL(struct drm_pvr_static_data_area, offset)
 
+/*
+ * Value of DRM_PVR_HEAP_COUNT in the first released version of enum
+ * drm_pvr_heap_id for Rogue cores.
+ */
+#define DRM_PVR_HEAP_COUNT_MIN_SUPPORTED (DRM_PVR_HEAP_TRANSFER_FRAG + 1)
+
+/*
+ * Count of static areas in the first released version of struct
+ * drm_pvr_static_data_area static_data_areas[] for Rogue cores.
+ */
+#define DRM_PVR_STATIC_DATA_AREA_COUNT_MIN_SUPPORTED 5U
+
 /**
  * PVR_UOBJ_GET() - Copies from _src_usr_ptr to &_dest_obj.
  * @_dest_obj: The destination container object in kernel space.
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index a369d73c6969..c51c162cd969 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -927,6 +927,9 @@ static const struct drm_pvr_static_data_area static_data_areas[] = {
 /*
  * The values given to GET_RESERVED_SIZE() are taken from the last entry in the corresponding
  * static data area for each heap.
+ *
+ * The pvr_heaps array **must** initialize **all** heaps that are enumerated in the UAPI with
+ * enum drm_pvr_heap_id.
  */
 static const struct drm_pvr_heap pvr_heaps[] = {
 	[DRM_PVR_HEAP_GENERAL] = {
@@ -967,6 +970,8 @@ static const struct drm_pvr_heap pvr_heaps[] = {
 	},
 };
 
+static_assert(ARRAY_SIZE(pvr_heaps) == DRM_PVR_HEAP_COUNT, "Invalid number of PVR heaps");
+
 int
 pvr_static_data_areas_get(const struct pvr_device *pvr_dev,
 			  struct drm_pvr_ioctl_dev_query_args *args)
@@ -985,13 +990,18 @@ pvr_static_data_areas_get(const struct pvr_device *pvr_dev,
 
 	if (!query.static_data_areas.array) {
 		u32 *stride = &query.static_data_areas.stride;
+		u32 *count = &query.static_data_areas.count;
 
 		if (!query.static_data_areas.stride)
 			*stride = DRM_PVR_STATIC_DATA_AREA_STRIDE_MIN_SUPPORTED;
 		else
 			*stride = sizeof(struct drm_pvr_static_data_area);
 
-		query.static_data_areas.count = ARRAY_SIZE(static_data_areas);
+		if (!query.static_data_areas.count)
+			*count = DRM_PVR_STATIC_DATA_AREA_COUNT_MIN_SUPPORTED;
+		else
+			*count = ARRAY_SIZE(static_data_areas);
+
 		goto copy_out;
 	}
 
@@ -1035,7 +1045,11 @@ pvr_heap_info_get(const struct pvr_device *pvr_dev,
 		else
 			query.heaps.stride = sizeof(struct drm_pvr_heap);
 
-		query.heaps.count = ARRAY_SIZE(pvr_heaps);
+		if (!query.heaps.count)
+			query.heaps.count = DRM_PVR_HEAP_COUNT_MIN_SUPPORTED;
+		else
+			query.heaps.count = ARRAY_SIZE(pvr_heaps);
+
 		goto copy_out;
 	}
 
diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
index fd7cf9e230c3..66a2af192db3 100644
--- a/include/uapi/drm/pvr_drm.h
+++ b/include/uapi/drm/pvr_drm.h
@@ -322,8 +322,13 @@ struct drm_pvr_dev_query_heap_info {
 	 * @heaps: Array of &struct drm_pvr_heap. To facilitate allocation by
 	 * the caller:
 	 *
-	 * - if &heaps.array is NULL, the count will be updated with the count
-	 *   of heaps known to the kernel driver.
+	 * - if &heaps.array is NULL and &heaps.count is set by the caller, the
+	 *   count will be updated with the value of @DRM_PVR_HEAP_COUNT that
+	 *   is known to the kernel driver.
+	 *
+	 * - if &heaps.array is NULL and &heaps.count is zero, the count will
+	 *   be updated with a value that is the minimum supported by the kernel
+	 *   and this may differ based on the GPU ID.
 	 *
 	 * - If &heaps.array is NULL and &heaps.stride is set by the caller, the
 	 *   stride will be updated to the size of &struct drm_pvr_heap known to
@@ -332,6 +337,11 @@ struct drm_pvr_dev_query_heap_info {
 	 * - If &heaps.array is NULL and &heaps.stride is zero, the stride will
 	 *   be updated with a value that is the minimum supported by the kernel
 	 *   and this may differ based on the GPU ID.
+	 *
+	 * - If &heaps.array, &heaps.stride and &heaps.count are set by the
+	 *   caller, then the kernel will try to honor the value of count and
+	 *   this may be clamped to the value of @DRM_PVR_HEAP_COUNT known by
+	 *   the kernel.
 	 */
 	struct drm_pvr_obj_array heaps;
 };
@@ -440,8 +450,13 @@ struct drm_pvr_dev_query_static_data_areas {
 	 * @static_data_areas: Array of &struct drm_pvr_static_data_area. To
 	 * facilitate allocation by the caller:
 	 *
-	 * - if &static_data_areas.array is NULL, the count will be updated with
-	 *   the count of static areas known to the kernel driver.
+	 * - if &static_data_areas.array is NULL and &static_data_areas.count is
+	 *   set by the caller, the count will be updated with the maximum count
+	 *   that is known to the kernel driver.
+	 *
+	 * - If &static_data_areas.array is NULL and &static_data_areas.count is
+	 *   zero, the count will be updated with a value that is the minimum
+	 *   supported by the kernel and this may differ based on the GPU ID.
 	 *
 	 * - If &static_data_areas.array is NULL and &static_data_areas.stride
 	 *   is set by the caller, the stride will be updated to the size of
@@ -451,6 +466,11 @@ struct drm_pvr_dev_query_static_data_areas {
 	 *   is zero, the stride will be updated with a value that is the
 	 *   minimum supported by the kernel and this may differ based on the
 	 *   GPU ID.
+	 *
+	 * - If &static_data_areas.array, &static_data_areas.stride and
+	 *   &static_data_areas.count are set by the caller, then the kernel
+	 *   will try to honor the value of count and this may be clamped to the
+	 *   maximum supported by the kernel.
 	 */
 	struct drm_pvr_obj_array static_data_areas;
 };

-- 
2.43.0