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(+)
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;
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;
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)
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
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
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
dri-devel@lists.freedesktop.org
-
cohens＠codeaurora.org -
Daniel Vetter -
Steve Cohen