diff mbox series

[1/2] drm/bridge: parade-ps8640: Never increase the length when reading from AUX

Message ID 20231211165526.1.I9d1afcaad76a3e2c0ca046dc4adbc2b632c22eda@changeid
State New
Headers
Series [1/2] drm/bridge: parade-ps8640: Never increase the length when reading from AUX |

Commit Message

Doug Anderson Dec. 12, 2023, 12:55 a.m. UTC 
  While testing, I happened to notice a random crash that looked like:

  Kernel panic - not syncing: stack-protector:
  Kernel stack is corrupted in: drm_dp_dpcd_probe+0x120/0x120

Analysis of drm_dp_dpcd_probe() shows that we pass in a 1-byte buffer
(allocated on the stack) to the aux->transfer() function. Presumably
if the aux->transfer() writes more than one byte to this buffer then
we're in a bad shape.

Dropping into kgdb, I noticed that "aux->transfer" pointed at
ps8640_aux_transfer().

Reading through ps8640_aux_transfer(), I can see that there are cases
where it could write more bytes to msg->buffer than were specified by
msg->size. This could happen if the hardware reported back something
bogus to us. Let's fix this so we never increase the length.

NOTE: I have no actual way to reproduce this issue but it seems likely
this is what was happening in the crash I looked at.

Fixes: 13afcdd7277e ("drm/bridge: parade-ps8640: Add support for AUX channel")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stephen Boyd Dec. 14, 2023, 1:05 a.m. UTC | #1 
Quoting Douglas Anderson (2023-12-11 16:55:26)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 8161b1a1a4b1..fb2ec4264549 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -302,7 +302,7 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
>
>                 fallthrough;
>         case SWAUX_STATUS_ACKM:
> -               len = data & SWAUX_M_MASK;
> +               len = min(len, (unsigned int)(data & SWAUX_M_MASK));
>                 break;
>         case SWAUX_STATUS_DEFER:
>         case SWAUX_STATUS_I2C_DEFER:
> @@ -310,7 +310,7 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
>                         msg->reply |= DP_AUX_NATIVE_REPLY_DEFER;
>                 else
>                         msg->reply |= DP_AUX_I2C_REPLY_DEFER;
> -               len = data & SWAUX_M_MASK;
> +               len = min(len, (unsigned int)(data & SWAUX_M_MASK));
>                 break;
>         case SWAUX_STATUS_INVALID:
>                 return -EOPNOTSUPP;

If the hardware indicates the len is larger than the length of 'buf' do
we need to throw away reads of the fifo until we read the length that
we're told? I'm specifically looking at the read loop at the end of
ps8640_aux_transfer_msg() where it reads a byte at a time out of
'PAGE0_SWAUX_RDATA'. So maybe what we need to do is have 'buf_len' and
'len' and then return the min of the two at the end of the function but
only copy over 'buf_len' amount.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8161b1a1a4b1..fb2ec4264549 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -302,7 +302,7 @@  static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
 
 		fallthrough;
 	case SWAUX_STATUS_ACKM:
-		len = data & SWAUX_M_MASK;
+		len = min(len, (unsigned int)(data & SWAUX_M_MASK));
 		break;
 	case SWAUX_STATUS_DEFER:
 	case SWAUX_STATUS_I2C_DEFER:
@@ -310,7 +310,7 @@  static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
 			msg->reply |= DP_AUX_NATIVE_REPLY_DEFER;
 		else
 			msg->reply |= DP_AUX_I2C_REPLY_DEFER;
-		len = data & SWAUX_M_MASK;
+		len = min(len, (unsigned int)(data & SWAUX_M_MASK));
 		break;
 	case SWAUX_STATUS_INVALID:
 		return -EOPNOTSUPP;