| Message ID | 20231211165526.1.I9d1afcaad76a3e2c0ca046dc4adbc2b632c22eda@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/bridge: parade-ps8640: Never increase the length when reading from AUX | expand |
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 --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;
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(-)