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 <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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;
--
2.43.0.472.g3155946c3a-goog
For aux reads, the value `msg->size` indicates the size of the buffer
provided by `msg->buffer`. We should never in any circumstances write
more bytes to the buffer since it may overflow the buffer.
In the ti-sn65dsi86 driver there is one code path that reads the
transfer length from hardware. Even though it's never been seen to be
a problem, we should make extra sure that the hardware isn't
increasing the length since doing so would cause us to overrun the
buffer.
Fixes: 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux failures")
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5b8e1dfc458d..7ff465241267 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -527,6 +527,7 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
u32 request_val = AUX_CMD_REQ(msg->request);
u8 *buf = msg->buffer;
unsigned int len = msg->size;
+ unsigned int short_len;
unsigned int val;
int ret;
u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
@@ -600,7 +601,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
}
if (val & AUX_IRQ_STATUS_AUX_SHORT) {
- ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
+ ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &short_len);
+ len = min(len, short_len);
if (ret)
goto exit;
} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
--
2.43.0.472.g3155946c3a-goog
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.