[PATCH 0/5] drm/dumb-buffers: Fix and improve buffer-size calculation

[PATCH 0/5] drm/dumb-buffers: Fix and improve buffer-size calculation

[Thomas Zimmermann]

Dumb-buffer pitch and size is specified by width, height, bits-per-pixel
plus various hardware-specific alignments. The calculation of these
values is inconsistent and duplicated among drivers. The results for
formats with bpp < 8 are incorrect.

This series begins to fix this. Default scanline pitch and buffer size
are now calculated with the existing 4CC helpers. The results are
provided to drivers to avoid recalculating them. The series fixes the
3 common GEM implementations for DMA, SHMEM and VRAM. Other memory
managers can later be addressed separately.

Thomas Zimmermann (5):
  drm/dumb-buffers: Sanitize output on errors
  drm/dumb-buffers: Fix size calculations and set default pitch and size
  drm/gem-dma: Use aligned default pitch and size for dumb buffers
  drm/gem-shmem: Use aligned default pitch and size for dumb buffers
  drm/gem-vram: Use default pitch and size for dumb buffers

 drivers/gpu/drm/drm_dumb_buffers.c       | 123 +++++++++++++++++------
 drivers/gpu/drm/drm_gem_dma_helper.c     |   7 +-
 drivers/gpu/drm/drm_gem_shmem_helper.c   |  16 +--
 drivers/gpu/drm/drm_gem_vram_helper.c    |  24 ++++-
 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c |   2 +
 include/drm/drm_dumb_buffers.h           |  12 +++
 6 files changed, 141 insertions(+), 43 deletions(-)
 create mode 100644 include/drm/drm_dumb_buffers.h

-- 
2.47.0

[PATCH 1/5] drm/dumb-buffers: Sanitize output on errors

[Thomas Zimmermann]

The ioctls MODE_CREATE_DUMB and MODE_MAP_DUMB return results into a
memory buffer supplied by user space. On errors, it is possible that
intermediate values are being returned. The exact semantics depends
on the DRM driver's implementation of these ioctls. Although this is
most-likely not a security problem in practice, avoid any uncertainty
by clearing the memory to 0 on errors.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_dumb_buffers.c | 40 ++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 70032bba1c97..9916aaf5b3f2 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -99,7 +99,30 @@ int drm_mode_create_dumb(struct drm_device *dev,
 int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 			       void *data, struct drm_file *file_priv)
 {
-	return drm_mode_create_dumb(dev, data, file_priv);
+	struct drm_mode_create_dumb *args = data;
+	int err;
+
+	err = drm_mode_create_dumb(dev, args, file_priv);
+	if (err) {
+		args->handle = 0;
+		args->pitch = 0;
+		args->size = 0;
+	}
+	return err;
+}
+
+static int drm_mode_mmap_dumb(struct drm_device *dev, struct drm_mode_map_dumb *args,
+			      struct drm_file *file_priv)
+{
+	if (!dev->driver->dumb_create)
+		return -ENOSYS;
+
+	if (dev->driver->dumb_map_offset)
+		return dev->driver->dumb_map_offset(file_priv, dev, args->handle,
+						    &args->offset);
+	else
+		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
+					       &args->offset);
 }
 
 /**
@@ -120,17 +143,12 @@ int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_map_dumb *args = data;
+	int err;
 
-	if (!dev->driver->dumb_create)
-		return -ENOSYS;
-
-	if (dev->driver->dumb_map_offset)
-		return dev->driver->dumb_map_offset(file_priv, dev,
-						    args->handle,
-						    &args->offset);
-	else
-		return drm_gem_dumb_map_offset(file_priv, dev, args->handle,
-					       &args->offset);
+	err = drm_mode_mmap_dumb(dev, args, file_priv);
+	if (err)
+		args->offset = 0;
+	return err;
 }
 
 int drm_mode_destroy_dumb(struct drm_device *dev, u32 handle,
-- 
2.47.0

[PATCH 2/5] drm/dumb-buffers: Fix size calculations and set default pitch and size

[Thomas Zimmermann]

Calculate the dumb-buffer scanline pitch with existing 4CC format
helpers and provide results to drivers. Fixes the overflow and size
tests. Drivers can further reuse the computed values.

The dumb-buffer overflow tests round up any given bits-per-pixel
value to a multiple of 8. So even one-bit formats, such as DRM_FORMAT_C1,
require 8 bits per pixel. While not common, low-end displays use such
formats; with a possibly considerable overallocation of memory.

There's also quite a bit of code duplication among DRM's memory
managers. Each calculates scanline pitch and buffer size from the given
arguments. But the implementations are inconsistent in how they treat
alignment and format support. Therefore, provide the already-calculated
values for pitch and size to memory managers. Later patches will update
each to make use of them.

The commit adds drm_mode_align_dumb(), a helper to align a given dumb
buffer's pitch and size. It also exports the function for memory managers
to use.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_dumb_buffers.c | 83 +++++++++++++++++++++++-------
 include/drm/drm_dumb_buffers.h     | 12 +++++
 2 files changed, 77 insertions(+), 18 deletions(-)
 create mode 100644 include/drm/drm_dumb_buffers.h

diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
index 9916aaf5b3f2..0f87a4c426c1 100644
--- a/drivers/gpu/drm/drm_dumb_buffers.c
+++ b/drivers/gpu/drm/drm_dumb_buffers.c
@@ -25,6 +25,8 @@
 
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
+#include <drm/drm_fourcc.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_mode.h>
 
@@ -57,41 +59,86 @@
  * a hardware-specific ioctl to allocate suitable buffer objects.
  */
 
+int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
+			unsigned long pitch_align,
+			unsigned long size_align)
+{
+	u32 pitch = args->pitch;
+	u32 size;
+
+	if (!pitch)
+		return -EINVAL;
+
+	if (pitch_align)
+		pitch = roundup(pitch, pitch_align);
+
+	/* overflow checks for 32bit size calculations */
+	if (args->height > U32_MAX / pitch)
+		return -EINVAL;
+
+	if (!size_align)
+		size_align = PAGE_SIZE;
+	else if (!IS_ALIGNED(size_align, PAGE_SIZE))
+		return -EINVAL;
+
+	size = ALIGN(args->height * pitch, size_align);
+	if (!size)
+		return -EINVAL;
+
+	args->pitch = pitch;
+	args->size = size;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_align_dumb);
+
 int drm_mode_create_dumb(struct drm_device *dev,
 			 struct drm_mode_create_dumb *args,
 			 struct drm_file *file_priv)
 {
-	u32 cpp, stride, size;
+	u32 fourcc;
+	const struct drm_format_info *info;
+	u64 pitch;
+	int ret;
 
 	if (!dev->driver->dumb_create)
 		return -ENOSYS;
 	if (!args->width || !args->height || !args->bpp)
 		return -EINVAL;
 
-	/* overflow checks for 32bit size calculations */
-	if (args->bpp > U32_MAX - 8)
-		return -EINVAL;
-	cpp = DIV_ROUND_UP(args->bpp, 8);
-	if (cpp > U32_MAX / args->width)
+	/*
+	 * The scanline pitch depends on the buffer width and the color
+	 * format. The latter is specified as a color-mode constant for
+	 * which we first have to find the corresponding color format.
+	 *
+	 * Different color formats can have the same color-mode constant.
+	 * For example XRGB8888 and BGRX8888 both have a color mode of 32.
+	 * It is possible to use different formats for dumb-buffer allocation
+	 * and rendering as long as all involved formats share the same
+	 * color-mode constant.
+	 */
+	fourcc = drm_driver_color_mode_format(dev, args->bpp);
+	if (fourcc == DRM_FORMAT_INVALID)
 		return -EINVAL;
-	stride = cpp * args->width;
-	if (args->height > U32_MAX / stride)
+	info = drm_format_info(fourcc);
+	if (!info)
 		return -EINVAL;
-
-	/* test for wrap-around */
-	size = args->height * stride;
-	if (PAGE_ALIGN(size) == 0)
+	pitch = drm_format_info_min_pitch(info, 0, args->width);
+	if (!pitch || pitch > U32_MAX)
 		return -EINVAL;
 
 	/*
-	 * handle, pitch and size are output parameters. Zero them out to
-	 * prevent drivers from accidentally using uninitialized data. Since
-	 * not all existing userspace is clearing these fields properly we
-	 * cannot reject IOCTL with garbage in them.
+	 * The fields handle, pitch and size are output parameters. Zero out
+	 * handle to prevent drivers from accidentally using uninitialized
+	 * data. Set default pitch and size to the values computed for the
+	 * overflow tests. Driver are free to override them, the default values
+	 * are what most drivers want.
 	 */
 	args->handle = 0;
-	args->pitch = 0;
-	args->size = 0;
+	args->pitch = pitch;
+	ret = drm_mode_align_dumb(args, 0, 0);
+	if (ret)
+		return ret;
 
 	return dev->driver->dumb_create(file_priv, dev, args);
 }
diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
new file mode 100644
index 000000000000..00172c5997d2
--- /dev/null
+++ b/include/drm/drm_dumb_buffers.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __DRM_DUMB_BUFFERS_H__
+#define __DRM_DUMB_BUFFERS_H__
+
+struct drm_mode_create_dumb;
+
+int drm_mode_align_dumb(struct drm_mode_create_dumb *args,
+			unsigned long pitch_align,
+			unsigned long size_align);
+
+#endif
-- 
2.47.0

[PATCH 3/5] drm/gem-dma: Use aligned default pitch and size for dumb buffers

[Thomas Zimmermann]

Use the pitch and size values stored in the args parameter for
allocating a dumb buffer in drm_gem_dma_dumb_create(). The values
come from drm_mode_create_dumb(). Align the pitch to a multiple
of 8.

Push the current calculation into the only direct caller imx. Imx's
hardware requires the framebuffer width to be aligned to 8. The
driver's current approach is actually incorrect, as it only guarantees
this implicitly and requires bpp to be a multiple of 8 already. A
later commit will fix this problem by aligning the scanline pitch
such that an aligned width still fits into each scanline's memory.

A number of other drivers are build on top of gem-dma helpers and
implement their own dumb-buffer allocation. These drivers invoke
drm_gem_dma_dumb_create_internal(), which is not affected by this
commit.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_dma_helper.c     | 7 +++++--
 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
index 870b90b78bc4..0961e52893b9 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -20,6 +20,7 @@
 #include <drm/drm.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_vma_manager.h>
 
@@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv,
 			    struct drm_mode_create_dumb *args)
 {
 	struct drm_gem_dma_object *dma_obj;
+	int ret;
 
-	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
-	args->size = args->pitch * args->height;
+	ret = drm_mode_align_dumb(args, 8, 0);
+	if (ret)
+		return ret;
 
 	dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size,
 						 &args->handle);
diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
index ced06bd8eae8..b3c9e8fcb45e 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
@@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv,
 	int ret;
 
 	args->width = ALIGN(width, 8);
+	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	args->size = args->pitch * args->height;
 
 	ret = drm_gem_dma_dumb_create(file_priv, drm, args);
 	if (ret)
-- 
2.47.0

[PATCH 4/5] drm/gem-shmem: Use aligned default pitch and size for dumb buffers

[Thomas Zimmermann]

Use the pitch and size values stored in the args parameter for
allocating a dumb buffer in drm_gem_shmem_dumb_create(). The values
come from drm_mode_create_dumb(). Align the pitch to a multiple of 8.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8508060a1a95..630a4fad7af1 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -18,6 +18,7 @@
 #include <drm/drm.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_prime.h>
 #include <drm/drm_print.h>
@@ -514,18 +515,11 @@ EXPORT_SYMBOL(drm_gem_shmem_purge);
 int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 			      struct drm_mode_create_dumb *args)
 {
-	u32 min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	int ret;
 
-	if (!args->pitch || !args->size) {
-		args->pitch = min_pitch;
-		args->size = PAGE_ALIGN(args->pitch * args->height);
-	} else {
-		/* ensure sane minimum values */
-		if (args->pitch < min_pitch)
-			args->pitch = min_pitch;
-		if (args->size < args->pitch * args->height)
-			args->size = PAGE_ALIGN(args->pitch * args->height);
-	}
+	ret = drm_mode_align_dumb(args, 8, 0);
+	if (ret)
+		return ret;
 
 	return drm_gem_shmem_create_with_handle(file, dev, args->size, &args->handle);
 }
-- 
2.47.0

[PATCH 5/5] drm/gem-vram: Use default pitch and size for dumb buffers

[Thomas Zimmermann]

Use the pitch and size values stored in the args parameter for
allocating a dumb buffer in drm_gem_vram_dumb_create(). Inline
the relevant code from drm_gem_vram_fill_create_dumb(), but
without the size computation. This value comes from
drm_mode_create_dumb(). Align the pitch to a multiple of 8.

Only hibmc and vboxvideo use gem-vram. Hibmc invokes the call to
drm_gem_vram_fill_create_dumb() directly and is therefore not affected.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 22b1fe9c03b8..0d1a9cce47e7 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -6,6 +6,7 @@
 #include <drm/drm_debugfs.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_dumb_buffers.h>
 #include <drm/drm_file.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -582,10 +583,31 @@ int drm_gem_vram_driver_dumb_create(struct drm_file *file,
 				    struct drm_device *dev,
 				    struct drm_mode_create_dumb *args)
 {
+	struct drm_gem_vram_object *gbo;
+	int ret;
+
 	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
 		return -EINVAL;
 
-	return drm_gem_vram_fill_create_dumb(file, dev, 0, 0, args);
+	ret = drm_mode_align_dumb(args, 8, 0);
+	if (ret)
+		return ret;
+
+	gbo = drm_gem_vram_create(dev, args->size, 0);
+	if (IS_ERR(gbo))
+		return PTR_ERR(gbo);
+
+	ret = drm_gem_handle_create(file, &gbo->bo.base, &args->handle);
+	if (ret)
+		goto err_drm_gem_object_put;
+
+	drm_gem_object_put(&gbo->bo.base);
+
+	return 0;
+
+err_drm_gem_object_put:
+	drm_gem_object_put(&gbo->bo.base);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_dumb_create);
 
-- 
2.47.0