Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Memory leak of the EDID buffer in `ptn3460_edid_read()` on successful reads. - [High] Race condition in bridge power state tracking (`ptn_bridge->enabled`). - [High] Improper error handling of short I2C transfers leads to kernel memory disclosure. -- commit 4e9f35dc1b32e95bb7e2572ee3b691a3841b8e02 Author: Piyush Patle <[email protected]> drm/bridge: nxp-ptn3460: replace deprecated DRM_ERROR macros This commit replaces DRM_ERROR() with device-aware equivalents like drm_err() and dev_err() throughout the nxp-ptn3460 bridge driver. > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c > b/drivers/gpu/drm/bridge/nxp-ptn3460.c > index 7acb11f16dc19..6f09c1247d7d8 100644 > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > @@ -54,13 +54,15 @@ static int ptn3460_read_bytes(struct ptn3460_bridge > *ptn_bridge, char addr, > > ret = i2c_master_send(ptn_bridge->client, &addr, 1); > if (ret < 0) { > - DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > + dev_err(&ptn_bridge->client->dev, > + "Failed to send i2c command, ret=%d\n", ret); > return ret; > } > > ret = i2c_master_recv(ptn_bridge->client, buf, len); > if (ret < 0) { > - DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); > + dev_err(&ptn_bridge->client->dev, > + "Failed to recv i2c data, ret=%d\n", ret); > return ret; > } [Severity: High] This is a pre-existing issue, but does this properly handle short I2C transfers? The i2c_master_recv() function returns the positive number of bytes transferred on success. If a short read occurs (e.g. the hardware NACKs mid-transfer), the return value is positive but less than len. Because the check is only for ret < 0, a short read is incorrectly treated as a full success, which leaves the target buffer partially uninitialized. In ptn3460_edid_read(), this uninitialized memory is then copied by drm_edid_alloc() and exposed to userspace, which could leak kernel memory. > > @@ -78,7 +80,8 @@ static int ptn3460_write_byte(struct ptn3460_bridge > *ptn_bridge, char addr, > > ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); > if (ret < 0) { > - DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); > + dev_err(&ptn_bridge->client->dev, > + "Failed to send i2c command, ret=%d\n", ret); > return ret; > } [Severity: High] This is a pre-existing issue, but does this similarly miss short writes? Like i2c_master_recv(), i2c_master_send() returns the number of bytes transferred. If this is a short write, ret will be positive but less than the requested size. Ignoring a short write could cause silent hardware configuration failures. [ ... ] > @@ -166,10 +171,8 @@ static const struct drm_edid *ptn3460_edid_read(struct > drm_bridge *bridge, > ptn3460_pre_enable(&ptn_bridge->bridge); [Severity: High] This is a pre-existing issue, but is the bridge power state tracking subject to a race condition here? In ptn3460_edid_read(), ptn_bridge->enabled is read and then updated by ptn3460_pre_enable() without locking. Because ptn3460_edid_read() runs under connector->mutex while the atomic commit tail runs bridge callbacks asynchronously, a concurrent atomic modeset could disable the bridge while ptn3460_edid_read() is actively performing I2C transactions. Could this lead to hardware lockups and I2C bus hangs? > > edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > - if (!edid) { > - DRM_ERROR("Failed to allocate EDID\n"); > + if (!edid) > goto out; > - } > > ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, > EDID_LENGTH); [Severity: High] This is a pre-existing issue, but does this function leak the edid buffer on the success path? Looking at ptn3460_edid_read() after the I2C read succeeds: drm_edid = drm_edid_alloc(edid, EDID_LENGTH); out: if (power_off) ptn3460_disable(&ptn_bridge->bridge); return drm_edid; The drm_edid_alloc() function uses kmemdup() to create its own copy of the memory. Since there is no kfree(edid) before returning on success, the original edid buffer appears to be leaked. Since userspace can repeatedly trigger EDID reads, could this lead to an unbounded memory leak and kernel memory exhaustion? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
