public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Johan Hovold <johan+linaro@kernel.org>
To: Douglas Anderson <dianders@chromium.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Rob Clark <robdclark@gmail.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Robert Foss <robert.foss@linaro.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Sean Paul <sean@poorly.run>, Stephen Boyd <swboyd@chromium.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Johan Hovold <johan+linaro@kernel.org>
Subject: [PATCH 0/7] drm/msm: probe deferral fixes
Date: Mon, 12 Sep 2022 17:40:39 +0200	[thread overview]
Message-ID: <20220912154046.12900-1-johan+linaro@kernel.org> (raw)

The MSM DRM is currently broken in multiple ways with respect to probe
deferral. Not only does the driver currently fail to probe again after a
late deferral, but due to a related use-after-free bug this also
triggers NULL-pointer dereferences.

These bugs are not new but have become critical with the release of
5.19 where probe is deferred in case the aux-bus EP panel driver has not
yet been loaded.

The underlying problem is lifetime issues due to careless use of
device-managed resources.

Specifically, device-managed resources allocated post component bind
must be tied to the lifetime of the aggregate DRM device or they will
not necessarily be released when binding of the aggregate device is
deferred.

The following call chain and pseudo code serves as an illustration of
the problem:

 - platform_probe(pdev1)
   - dp_display_probe()
     - component_add()

 - platform_probe(pdev2) 				// last component
   - dp_display_probe()					// d0
       - component_add()
         - try_to_bring_up_aggregate_device()
	   - devres_open_group(adev->parent)		// d1

	   - msm_drm_bind()
	     - msm_drm_init()
	       - component_bind_all()
	         - for_each_component()
		   - component_bind()
		     - devres_open_group(&pdev->dev)	// d2
	             - dp_display_bind()
		       - devm_kzalloc(&pdev->dev)	// a1, OK
		     - devres_close_group(&pdev->dev)	// d3

	       - dpu_kms_hw_init()
	         - for_each_panel()
	           - msm_dp_modeset_init()
		     - dp_display_request_irq()
		       - devm_request_irq(&pdev->dev)	// a2, BUG
		     - if (pdev == pdev2 && condition)
		       - return -EPROBE_DEFER;

	      - if (error)
 	        - component_unbind_all()
	          - for_each_component()
		    - component_unbind()
		      - dp_display_unbind()
		      - devres_release_group(&pdev->dev) // d4, only a1 is freed

           - if (error)
	     - devres_release_group(adev->parent)	// d5

The device-managed allocation a2 is buggy as its lifetime is tied to the
component platform device and will not be released when the aggregate
device bind fails (e.g. due to a probe deferral).

When pdev2 is later probed again, the attempt to allocate the IRQ a
second time will fail for pdev1 (which is still bound to its platform
driver).

This series fixes the lifetime issues by tying the lifetime of a2 (and
similar allocations) to the lifetime of the aggregate device so that a2
is released at d5.

In some cases, such has for the DP IRQ, the above situation can also be
avoided by moving the allocation in question to the platform driver
probe (d0) or component bind (between d2 and d3). But as doing so is not
a general fix, this can be done later as a cleanup/optimisation.

Johan


Johan Hovold (7):
  drm/msm: fix use-after-free on probe deferral
  drm/msm: fix memory corruption with too many bridges
  drm/msm/dp: fix IRQ lifetime
  drm/msm/dp: fix aux-bus EP lifetime
  drm/msm/dp: fix bridge lifetime
  drm/msm/hdmi: fix IRQ lifetime
  drm/msm: drop modeset sanity checks

 drivers/gpu/drm/bridge/parade-ps8640.c   |  2 +-
 drivers/gpu/drm/display/drm_dp_aux_bus.c |  5 +++--
 drivers/gpu/drm/msm/dp/dp_display.c      | 16 +++++++++-------
 drivers/gpu/drm/msm/dp/dp_parser.c       |  6 +++---
 drivers/gpu/drm/msm/dp/dp_parser.h       |  5 +++--
 drivers/gpu/drm/msm/dsi/dsi.c            |  9 +++++----
 drivers/gpu/drm/msm/hdmi/hdmi.c          |  7 ++++++-
 drivers/gpu/drm/msm/msm_drv.c            |  1 +
 include/drm/display/drm_dp_aux_bus.h     |  6 +++---
 9 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.35.1


             reply	other threads:[~2022-09-12 15:41 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-12 15:40 Johan Hovold [this message]
2022-09-12 15:40 ` [PATCH 1/7] drm/msm: fix use-after-free on probe deferral Johan Hovold
2022-09-12 17:52   ` Dmitry Baryshkov
2022-09-13  7:31     ` Johan Hovold
2022-09-12 15:40 ` [PATCH 2/7] drm/msm: fix memory corruption with too many bridges Johan Hovold
2022-09-12 17:55   ` Dmitry Baryshkov
2022-09-13  7:49     ` Johan Hovold
2022-09-12 15:40 ` [PATCH 3/7] drm/msm/dp: fix IRQ lifetime Johan Hovold
2022-09-12 18:06   ` Dmitry Baryshkov
2022-09-12 15:40 ` [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime Johan Hovold
2022-09-12 18:10   ` Dmitry Baryshkov
2022-09-12 21:55     ` Steev Klimaszewski
2022-09-13  7:20       ` Johan Hovold
2022-09-13  6:35     ` Doug Anderson
2022-09-13  7:18       ` Johan Hovold
2022-09-13  1:51   ` kernel test robot
2022-09-12 15:40 ` [PATCH 5/7] drm/msm/dp: fix bridge lifetime Johan Hovold
2022-09-12 18:11   ` Dmitry Baryshkov
2022-09-12 15:40 ` [PATCH 6/7] drm/msm/hdmi: fix IRQ lifetime Johan Hovold
2022-09-12 17:59   ` Dmitry Baryshkov
2022-09-12 15:40 ` [PATCH 7/7] drm/msm: drop modeset sanity checks Johan Hovold
2022-09-12 18:06   ` Dmitry Baryshkov
2022-09-13  7:53     ` Johan Hovold
2022-09-12 17:11 ` [PATCH 0/7] drm/msm: probe deferral fixes Abhinav Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220912154046.12900-1-johan+linaro@kernel.org \
    --to=johan+linaro@kernel.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=airlied@linux.ie \
    --cc=andersson@kernel.org \
    --cc=andrzej.hajda@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=dianders@chromium.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=neil.armstrong@linaro.org \
    --cc=robdclark@gmail.com \
    --cc=robert.foss@linaro.org \
    --cc=sean@poorly.run \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox