[PATCH v2 0/3] drm/mst: Fix sideband msg reception error path

[PATCH v2 0/3] drm/mst: Fix sideband msg reception error path

[Imre Deak]

This is a resend of the last two patches from [1], addressing Lyude's
comments and adding his R-bs. The first patch in [1] isn't needed any
more. Patch 3 is a fix for a related issue I noticed since the original
patchset.

https://lists.freedesktop.org/archives/dri-devel/2016-May/107420.html

Cc: Dave Airlie <airlied@redhat.com>
Cc: Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Imre Deak (3):
  drm/mst: Fix error handling during MST sideband message reception
  drm/mst: Avoid dereferencing a NULL mstb in drm_dp_mst_handle_up_req()
  drm/mst: Avoid processing partially received up/down message
    transactions

 drivers/gpu/drm/drm_dp_mst_topology.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

-- 
2.13.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm/mst: Fix error handling during MST sideband message reception

[Imre Deak]

Handle any error due to partial reads, timeouts etc. to avoid parsing
uninitialized data subsequently. Also bail out if the parsing itself
fails.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Lyude <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index f7e292bf2baf..c6ae3e68918f 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2200,11 +2200,17 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 		ret = drm_dp_dpcd_read(mgr->aux, basereg + curreply,
 				    replyblock, len);
 		if (ret != len) {
-			DRM_DEBUG_KMS("failed to read a chunk\n");
+			DRM_DEBUG_KMS("failed to read a chunk (len %d, ret %d)\n",
+				      len, ret);
+			return;
 		}
+
 		ret = drm_dp_sideband_msg_build(msg, replyblock, len, false);
-		if (ret == false)
+		if (!ret) {
 			DRM_DEBUG_KMS("failed to build sideband msg\n");
+			return;
+		}
+
 		curreply += len;
 		replylen -= len;
 	}
-- 
2.13.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/mst: Avoid dereferencing a NULL mstb in drm_dp_mst_handle_up_req()

[Imre Deak]

In case of an unknown broadcast message is sent mstb will remain unset,
so check for this.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Lyude <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index c6ae3e68918f..78e9a7d58794 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2328,7 +2328,9 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
 			DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n", msg.u.resource_stat.port_number, msg.u.resource_stat.available_pbn);
 		}
 
-		drm_dp_put_mst_branch_device(mstb);
+		if (mstb)
+			drm_dp_put_mst_branch_device(mstb);
+
 		memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx));
 	}
 	return ret;
-- 
2.13.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/mst: Avoid processing partially received up/down message transactions

[Imre Deak]

Currently we may process up/down message transactions containing
uninitialized data. This can happen if there was an error during the
reception of any message in the transaction, but we happened to receive
the last message correctly with the end-of-message flag set.

To avoid this abort the reception of the transaction when the first
error is detected, rejecting any messages until a message with the
start-of-message flag is received (which will start a new transaction).
This is also what the DP 1.4 spec 2.11.8.2 calls for in this case.

In addtion this also prevents receiving bogus transactions without the
first message with the the start-of-message flag set.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 78e9a7d58794..25ed0ef531d4 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2168,7 +2168,7 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr)
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
 
-static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
+static bool drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 {
 	int len;
 	u8 replyblock[32];
@@ -2183,12 +2183,12 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 			       replyblock, len);
 	if (ret != len) {
 		DRM_DEBUG_KMS("failed to read DPCD down rep %d %d\n", len, ret);
-		return;
+		return false;
 	}
 	ret = drm_dp_sideband_msg_build(msg, replyblock, len, true);
 	if (!ret) {
 		DRM_DEBUG_KMS("sideband msg build failed %d\n", replyblock[0]);
-		return;
+		return false;
 	}
 	replylen = msg->curchunk_len + msg->curchunk_hdrlen;
 
@@ -2202,25 +2202,30 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 		if (ret != len) {
 			DRM_DEBUG_KMS("failed to read a chunk (len %d, ret %d)\n",
 				      len, ret);
-			return;
+			return false;
 		}
 
 		ret = drm_dp_sideband_msg_build(msg, replyblock, len, false);
 		if (!ret) {
 			DRM_DEBUG_KMS("failed to build sideband msg\n");
-			return;
+			return false;
 		}
 
 		curreply += len;
 		replylen -= len;
 	}
+	return true;
 }
 
 static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 {
 	int ret = 0;
 
-	drm_dp_get_one_sb_msg(mgr, false);
+	if (!drm_dp_get_one_sb_msg(mgr, false)) {
+		memset(&mgr->down_rep_recv, 0,
+		       sizeof(struct drm_dp_sideband_msg_rx));
+		return 0;
+	}
 
 	if (mgr->down_rep_recv.have_eomt) {
 		struct drm_dp_sideband_msg_tx *txmsg;
@@ -2276,7 +2281,12 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
 {
 	int ret = 0;
-	drm_dp_get_one_sb_msg(mgr, true);
+
+	if (!drm_dp_get_one_sb_msg(mgr, true)) {
+		memset(&mgr->up_req_recv, 0,
+		       sizeof(struct drm_dp_sideband_msg_rx));
+		return 0;
+	}
 
 	if (mgr->up_req_recv.have_eomt) {
 		struct drm_dp_sideband_msg_req_body msg;
-- 
2.13.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/3] drm/mst: Avoid processing partially received up/down message transactions

[Imre Deak]

Currently we may process up/down message transactions containing
uninitialized data. This can happen if there was an error during the
reception of any message in the transaction, but we happened to receive
the last message correctly with the end-of-message flag set.

To avoid this abort the reception of the transaction when the first
error is detected, rejecting any messages until a message with the
start-of-message flag is received (which will start a new transaction).
This is also what the DP 1.4 spec 2.11.8.2 calls for in this case.

In addtion this also prevents receiving bogus transactions without the
first message with the the start-of-message flag set.

v2:
- unchanged
v3:
- git add the part that actually skips messages after an error in
  drm_dp_sideband_msg_build()

Cc: Dave Airlie <airlied@redhat.com>
Cc: Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 78e9a7d58794..41b492f99955 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -332,6 +332,13 @@ static bool drm_dp_sideband_msg_build(struct drm_dp_sideband_msg_rx *msg,
 			return false;
 		}
 
+		/*
+		 * ignore out-of-order messages or messages that are part of a
+		 * failed transaction
+		 */
+		if (!recv_hdr.somt && !msg->have_somt)
+			return false;
+
 		/* get length contained in this portion */
 		msg->curchunk_len = recv_hdr.msg_len;
 		msg->curchunk_hdrlen = hdrlen;
@@ -2168,7 +2175,7 @@ int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr)
 }
 EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
 
-static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
+static bool drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 {
 	int len;
 	u8 replyblock[32];
@@ -2183,12 +2190,12 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 			       replyblock, len);
 	if (ret != len) {
 		DRM_DEBUG_KMS("failed to read DPCD down rep %d %d\n", len, ret);
-		return;
+		return false;
 	}
 	ret = drm_dp_sideband_msg_build(msg, replyblock, len, true);
 	if (!ret) {
 		DRM_DEBUG_KMS("sideband msg build failed %d\n", replyblock[0]);
-		return;
+		return false;
 	}
 	replylen = msg->curchunk_len + msg->curchunk_hdrlen;
 
@@ -2202,25 +2209,30 @@ static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up)
 		if (ret != len) {
 			DRM_DEBUG_KMS("failed to read a chunk (len %d, ret %d)\n",
 				      len, ret);
-			return;
+			return false;
 		}
 
 		ret = drm_dp_sideband_msg_build(msg, replyblock, len, false);
 		if (!ret) {
 			DRM_DEBUG_KMS("failed to build sideband msg\n");
-			return;
+			return false;
 		}
 
 		curreply += len;
 		replylen -= len;
 	}
+	return true;
 }
 
 static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 {
 	int ret = 0;
 
-	drm_dp_get_one_sb_msg(mgr, false);
+	if (!drm_dp_get_one_sb_msg(mgr, false)) {
+		memset(&mgr->down_rep_recv, 0,
+		       sizeof(struct drm_dp_sideband_msg_rx));
+		return 0;
+	}
 
 	if (mgr->down_rep_recv.have_eomt) {
 		struct drm_dp_sideband_msg_tx *txmsg;
@@ -2276,7 +2288,12 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
 {
 	int ret = 0;
-	drm_dp_get_one_sb_msg(mgr, true);
+
+	if (!drm_dp_get_one_sb_msg(mgr, true)) {
+		memset(&mgr->up_req_recv, 0,
+		       sizeof(struct drm_dp_sideband_msg_rx));
+		return 0;
+	}
 
 	if (mgr->up_req_recv.have_eomt) {
 		struct drm_dp_sideband_msg_req_body msg;
-- 
2.13.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/3] drm/mst: Avoid processing partially received up/down message transactions

[Lyude Paul]

On Wed, 2017-07-19 at 16:46 +0300, Imre Deak wrote:
> Currently we may process up/down message transactions containing
> uninitialized data. This can happen if there was an error during the
> reception of any message in the transaction, but we happened to
> receive
> the last message correctly with the end-of-message flag set.
> 
> To avoid this abort the reception of the transaction when the first
> error is detected, rejecting any messages until a message with the
> start-of-message flag is received (which will start a new
> transaction).
> This is also what the DP 1.4 spec 2.11.8.2 calls for in this case.
> 
> In addtion this also prevents receiving bogus transactions without
> the
s/addtion/addition/

With the one small spelling fix:

Reviewed-by: Lyude <lyude@redhat.com>

> first message with the the start-of-message flag set.
> 
> v2:
> - unchanged
> v3:
> - git add the part that actually skips messages after an error in
>   drm_dp_sideband_msg_build()
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Lyude <lyude@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++-
> ------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 78e9a7d58794..41b492f99955 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -332,6 +332,13 @@ static bool drm_dp_sideband_msg_build(struct
> drm_dp_sideband_msg_rx *msg,
>  			return false;
>  		}
>  
> +		/*
> +		 * ignore out-of-order messages or messages that are
> part of a
> +		 * failed transaction
> +		 */
> +		if (!recv_hdr.somt && !msg->have_somt)
> +			return false;
> +
>  		/* get length contained in this portion */
>  		msg->curchunk_len = recv_hdr.msg_len;
>  		msg->curchunk_hdrlen = hdrlen;
> @@ -2168,7 +2175,7 @@ int drm_dp_mst_topology_mgr_resume(struct
> drm_dp_mst_topology_mgr *mgr)
>  }
>  EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
>  
> -static void drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr
> *mgr, bool up)
> +static bool drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr
> *mgr, bool up)
>  {
>  	int len;
>  	u8 replyblock[32];
> @@ -2183,12 +2190,12 @@ static void drm_dp_get_one_sb_msg(struct
> drm_dp_mst_topology_mgr *mgr, bool up)
>  			       replyblock, len);
>  	if (ret != len) {
>  		DRM_DEBUG_KMS("failed to read DPCD down rep %d
> %d\n", len, ret);
> -		return;
> +		return false;
>  	}
>  	ret = drm_dp_sideband_msg_build(msg, replyblock, len, true);
>  	if (!ret) {
>  		DRM_DEBUG_KMS("sideband msg build failed %d\n",
> replyblock[0]);
> -		return;
> +		return false;
>  	}
>  	replylen = msg->curchunk_len + msg->curchunk_hdrlen;
>  
> @@ -2202,25 +2209,30 @@ static void drm_dp_get_one_sb_msg(struct
> drm_dp_mst_topology_mgr *mgr, bool up)
>  		if (ret != len) {
>  			DRM_DEBUG_KMS("failed to read a chunk (len
> %d, ret %d)\n",
>  				      len, ret);
> -			return;
> +			return false;
>  		}
>  
>  		ret = drm_dp_sideband_msg_build(msg, replyblock,
> len, false);
>  		if (!ret) {
>  			DRM_DEBUG_KMS("failed to build sideband
> msg\n");
> -			return;
> +			return false;
>  		}
>  
>  		curreply += len;
>  		replylen -= len;
>  	}
> +	return true;
>  }
>  
>  static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr
> *mgr)
>  {
>  	int ret = 0;
>  
> -	drm_dp_get_one_sb_msg(mgr, false);
> +	if (!drm_dp_get_one_sb_msg(mgr, false)) {
> +		memset(&mgr->down_rep_recv, 0,
> +		       sizeof(struct drm_dp_sideband_msg_rx));
> +		return 0;
> +	}
>  
>  	if (mgr->down_rep_recv.have_eomt) {
>  		struct drm_dp_sideband_msg_tx *txmsg;
> @@ -2276,7 +2288,12 @@ static int drm_dp_mst_handle_down_rep(struct
> drm_dp_mst_topology_mgr *mgr)
>  static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr
> *mgr)
>  {
>  	int ret = 0;
> -	drm_dp_get_one_sb_msg(mgr, true);
> +
> +	if (!drm_dp_get_one_sb_msg(mgr, true)) {
> +		memset(&mgr->up_req_recv, 0,
> +		       sizeof(struct drm_dp_sideband_msg_rx));
> +		return 0;
> +	}
>  
>  	if (mgr->up_req_recv.have_eomt) {
>  		struct drm_dp_sideband_msg_req_body msg;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/mst: Fix sideband msg reception error path

[Lyude Paul]

On Wed, 2017-07-19 at 14:43 +0300, Imre Deak wrote:
> This is a resend of the last two patches from [1], addressing Lyude's
> comments and adding his R-bs. The first patch in [1] isn't needed any
It's "her" by the way :P.

Also would you mind resending this with a Cc to stable@vger.kernel.org
after seeing if this can be backported? This is probably something we
should get into the stable kernel as well.

> more. Patch 3 is a fix for a related issue I noticed since the
> original
> patchset.
> 
> https://lists.freedesktop.org/archives/dri-devel/2016-May/107420.html
> 
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Lyude <lyude@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Imre Deak (3):
>   drm/mst: Fix error handling during MST sideband message reception
>   drm/mst: Avoid dereferencing a NULL mstb in
> drm_dp_mst_handle_up_req()
>   drm/mst: Avoid processing partially received up/down message
>     transactions
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 34
> ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/mst: Fix sideband msg reception error path

[Daniel Vetter]

On Wed, Jul 19, 2017 at 02:16:35PM -0400, Lyude Paul wrote:
> Also would you mind resending this with a Cc to stable@vger.kernel.org
> after seeing if this can be backported? This is probably something we
> should get into the stable kernel as well.

Applied to drm-misc-fixes with cc: stable on all of them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/mst: Fix sideband msg reception error path

[Imre Deak]

On Wed, Jul 19, 2017 at 02:16:35PM -0400, Lyude Paul wrote:
> On Wed, 2017-07-19 at 14:43 +0300, Imre Deak wrote:
> > This is a resend of the last two patches from [1], addressing Lyude's
> > comments and adding his R-bs. The first patch in [1] isn't needed any
> It's "her" by the way :P.

Ok, didn't know, noted now:)

> Also would you mind resending this with a Cc to stable@vger.kernel.org
> after seeing if this can be backported?

Daniel did that already, I can follow-up if any backport is needed for
older stable versions.

Thanks for the review,
Imre

> This is probably something we should get into the stable kernel as
> well.
> 
> > more. Patch 3 is a fix for a related issue I noticed since the
> > original patchset.
> > 
> > https://lists.freedesktop.org/archives/dri-devel/2016-May/107420.html
> > 
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Lyude <lyude@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Imre Deak (3):
> >   drm/mst: Fix error handling during MST sideband message reception
> >   drm/mst: Avoid dereferencing a NULL mstb in
> > drm_dp_mst_handle_up_req()
> >   drm/mst: Avoid processing partially received up/down message
> >     transactions
> > 
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 34
> > ++++++++++++++++++++++++++--------
> >  1 file changed, 26 insertions(+), 8 deletions(-)
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel