[PATCH 0/3] allow DRM drivers to limit creation of blobs

[PATCH 0/3] allow DRM drivers to limit creation of blobs

[Steve Cohen]

Fuzzers used in Android compliance testing repeatedly call the
create blob IOCTL which eventually exhausts the system memory.
This series adds a hook which allows drivers to impose their own
limitations on the size and/or number of blobs created.

Steve Cohen (3):
  drm: add driver hook for create blob limitations
  drm/msm: add support for createblob_check driver hook
  drm/msm/dpu: check blob limitations during create blob ioctl

 drivers/gpu/drm/drm_property.c          |  7 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
 drivers/gpu/drm/msm/msm_drv.c           | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h           |  1 +
 include/drm/drm_drv.h                   |  9 +++++++++
 5 files changed, 56 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] drm: add driver hook for create blob limitations

[Steve Cohen]

Allow drivers with blob limitations to run checks before blobs
are created. This can be used to limit how much memory can be
allocated based on driver requirements.

Signed-off-by: Steve Cohen <cohens@codeaurora.org>
---
 drivers/gpu/drm/drm_property.c | 7 +++++++
 include/drm/drm_drv.h          | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 892ce63..507a8a1 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -793,6 +793,13 @@ int drm_mode_createblob_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	if (dev->driver->createblob_check) {
+		ret = dev->driver->createblob_check(
+				dev, out_resp->length, file_priv);
+		if (ret)
+			return ret;
+	}
+
 	blob = drm_property_create_blob(dev, out_resp->length, NULL);
 	if (IS_ERR(blob))
 		return PTR_ERR(blob);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 8976afe..73b39b89 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -776,6 +776,15 @@ struct drm_driver {
 	int (*dma_quiescent) (struct drm_device *);
 	int (*context_dtor) (struct drm_device *dev, int context);
 	int dev_priv_size;
+
+	/**
+	 * @createblob_check: driver check for creating blob properties
+	 *
+	 * Hook for checking blob limitations imposed by drivers
+	 */
+	int (*createblob_check) (struct drm_device *dev,
+				 size_t length,
+				 struct drm_file *file_priv);
 };
 
 extern unsigned int drm_debug;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] drm/msm: add support for createblob_check driver hook

[Steve Cohen]

Allow msm_kms devices to register a hook to check blob count
and blob size limitations before a new blob is created.

Signed-off-by: Steve Cohen <cohens@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c84f0a8..d0b0419 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -680,6 +680,30 @@ static void msm_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	vblank_ctrl_queue_work(priv, pipe, false);
 }
 
+static int msm_createblob_check (struct drm_device *dev, size_t length,
+		struct drm_file *file_priv)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+	unsigned int count = 0;
+	struct drm_property_blob *blob;
+
+	if (!kms)
+		return -EINVAL;
+
+	if (!kms->funcs->createblob_check)
+		return 0;
+
+	mutex_lock(&dev->mode_config.blob_lock);
+	list_for_each_entry(blob, &file_priv->blobs, head_file) {
+		if (count < UINT_MAX)
+			count++;
+	}
+	mutex_unlock(&dev->mode_config.blob_lock);
+
+	return kms->funcs->createblob_check(count, length);
+}
+
 /*
  * DRM ioctls:
  */
@@ -1011,6 +1035,7 @@ static struct drm_driver msm_driver = {
 	.gem_prime_vmap     = msm_gem_prime_vmap,
 	.gem_prime_vunmap   = msm_gem_prime_vunmap,
 	.gem_prime_mmap     = msm_gem_prime_mmap,
+	.createblob_check   = msm_createblob_check,
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init       = msm_debugfs_init,
 #endif
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 1cbef6b..8a7e581 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -126,6 +126,7 @@ struct msm_kms_funcs {
 	/* debugfs: */
 	int (*debugfs_init)(struct msm_kms *kms, struct drm_minor *minor);
 #endif
+	int (*createblob_check)(unsigned int count, size_t length);
 };
 
 struct msm_kms;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] drm/msm/dpu: check blob limitations during create blob ioctl

[Steve Cohen]

Limit the blob size and number of blobs that can be allocated
by a client. This prevents fuzzers from abusing this ioctl and
exhausting the system memory.

Signed-off-by: Steve Cohen <cohens@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6c92f0f..5fbb7c3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -41,6 +41,8 @@
  */
 #define DPU_DEBUGFS_DIR "msm_dpu"
 #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask"
+#define MAX_BLOB_PROP_SIZE	(PAGE_SIZE * 30)
+#define MAX_BLOB_PROP_COUNT	250
 
 static int dpu_kms_hw_init(struct msm_kms *kms);
 static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms);
@@ -544,6 +546,17 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 	return ret;
 }
 
+static int dpu_kms_createblob_check(unsigned int count, size_t length)
+{
+	if (count >= MAX_BLOB_PROP_COUNT)
+		return -EINVAL;
+
+	if (length > MAX_BLOB_PROP_SIZE)
+		return -EINVAL;
+
+	return 0;
+}
+
 static long dpu_kms_round_pixclk(struct msm_kms *kms, unsigned long rate,
 		struct drm_encoder *encoder)
 {
@@ -683,6 +696,7 @@ static const struct msm_kms_funcs kms_funcs = {
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init    = dpu_kms_debugfs_init,
 #endif
+	.createblob_check = dpu_kms_createblob_check,
 };
 
 static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 0/3] allow DRM drivers to limit creation of blobs

[Daniel Vetter]

On Thu, Nov 07, 2019 at 02:39:11PM -0500, Steve Cohen wrote:
> Fuzzers used in Android compliance testing repeatedly call the
> create blob IOCTL which eventually exhausts the system memory.
> This series adds a hook which allows drivers to impose their own
> limitations on the size and/or number of blobs created.

Pretty sure this isn't just a problem for msm/dpu alone, why this very
limited approach?

Also, why are your fuzzers not also allocating enormous amounts of gem
buffers, which will also exhaust memory eventually?
-Daniel

> 
> Steve Cohen (3):
>   drm: add driver hook for create blob limitations
>   drm/msm: add support for createblob_check driver hook
>   drm/msm/dpu: check blob limitations during create blob ioctl
> 
>  drivers/gpu/drm/drm_property.c          |  7 +++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
>  drivers/gpu/drm/msm/msm_drv.c           | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/msm_kms.h           |  1 +
>  include/drm/drm_drv.h                   |  9 +++++++++
>  5 files changed, 56 insertions(+)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] allow DRM drivers to limit creation of blobs

[cohens]

On 2019-11-08 03:34, Daniel Vetter wrote:
> On Thu, Nov 07, 2019 at 02:39:11PM -0500, Steve Cohen wrote:
>> Fuzzers used in Android compliance testing repeatedly call the
>> create blob IOCTL which eventually exhausts the system memory.
>> This series adds a hook which allows drivers to impose their own
>> limitations on the size and/or number of blobs created.
> 
> Pretty sure this isn't just a problem for msm/dpu alone, why this very
> limited approach?
> 
I'm not familiar enough with the blob requirements for other
vendor's drivers to impose any restrictions on them. The idea
was to provide the hook for vendors to implement their own
checks. Support for msm/mdp* drivers will be added in v2 if this
approach is acceptable.

> Also, why are your fuzzers not also allocating enormous amounts of gem
> buffers, which will also exhaust memory eventually?

Excellent question... This will likely come in a follow-up series.

> -Daniel
> 
>> 
>> Steve Cohen (3):
>>   drm: add driver hook for create blob limitations
>>   drm/msm: add support for createblob_check driver hook
>>   drm/msm/dpu: check blob limitations during create blob ioctl
>> 
>>  drivers/gpu/drm/drm_property.c          |  7 +++++++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
>>  drivers/gpu/drm/msm/msm_drv.c           | 25 
>> +++++++++++++++++++++++++
>>  drivers/gpu/drm/msm/msm_kms.h           |  1 +
>>  include/drm/drm_drv.h                   |  9 +++++++++
>>  5 files changed, 56 insertions(+)
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] allow DRM drivers to limit creation of blobs

[Daniel Vetter]

On Sat, Nov 9, 2019 at 1:01 AM <cohens@codeaurora.org> wrote:
>
> On 2019-11-08 03:34, Daniel Vetter wrote:
> > On Thu, Nov 07, 2019 at 02:39:11PM -0500, Steve Cohen wrote:
> >> Fuzzers used in Android compliance testing repeatedly call the
> >> create blob IOCTL which eventually exhausts the system memory.
> >> This series adds a hook which allows drivers to impose their own
> >> limitations on the size and/or number of blobs created.
> >
> > Pretty sure this isn't just a problem for msm/dpu alone, why this very
> > limited approach?
> >
> I'm not familiar enough with the blob requirements for other
> vendor's drivers to impose any restrictions on them. The idea
> was to provide the hook for vendors to implement their own
> checks. Support for msm/mdp* drivers will be added in v2 if this
> approach is acceptable.

Yeah, but I don't think different limits (and then maybe different
tunables for these different limits) on drivers isn't a great idea.
Plus I thought this kind of stuff was supposed to be catched by the
kernel memory cgroup controller. Does that not work? The blob stuff is
maybe a bit too direct way to allocate kernel memory, but there's many
others I've thought. This all just feels a lot like busywork to check
an android compliance checkbox, without really digging into the
underlying problem and fixing it for real.

One approach that would work I think would be:
- Extended the blob property type to have min/max size (we might need
a range for some), set it for all current blob properties. To do that
we'd need to create a new property creation function for blobs, which
takes those limits. There's not a hole lot of them, so should be
doable.
- In the create blob function we walk all property (or book-keep that
somewhere) and check the blob isn't too big.
- We also validate the size when setting the property, at least some
of that code from various places.

I think this would actually improve the situation here, instead of
whack-a-mole. The cheaper cope-out would be if we simply limit the
total property size to something big enough, but not unlimited, like
16MB or something like that.

> > Also, why are your fuzzers not also allocating enormous amounts of gem
> > buffers, which will also exhaust memory eventually?
>
> Excellent question... This will likely come in a follow-up series.

Would be good to know that, so we can solve this for real, not just a
one-off for the compliance checkbox. Also really wondering why cgroups
doesn't catch this.
-Daniel

>
> > -Daniel
> >
> >>
> >> Steve Cohen (3):
> >>   drm: add driver hook for create blob limitations
> >>   drm/msm: add support for createblob_check driver hook
> >>   drm/msm/dpu: check blob limitations during create blob ioctl
> >>
> >>  drivers/gpu/drm/drm_property.c          |  7 +++++++
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
> >>  drivers/gpu/drm/msm/msm_drv.c           | 25
> >> +++++++++++++++++++++++++
> >>  drivers/gpu/drm/msm/msm_kms.h           |  1 +
> >>  include/drm/drm_drv.h                   |  9 +++++++++
> >>  5 files changed, 56 insertions(+)
> >>
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum,
> >> a Linux Foundation Collaborative Project
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch