| Message ID | 20230721101600.4392-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/crtc: do not release uninitialized connector reference | expand |
On 23/07/21 01:15PM, Fedor Pchelkin wrote: > Inside drm_mode_setcrtc() connector_set is allocated using kmalloc_array() > so its values are uninitialized. When filling this array with actual > pointers to drm connector objects, an error caused with invalid ioctl > request data may occur leading us to put references to already taken > objects. However, the last elements of the array are left uninitialized > which makes drm_connector_put() to be called with an invalid argument. > > We can obviously just initialize the array with kcalloc() but the current > fix chose a slightly different way. > > The index of failing array element is known so just put references to the > array members with lower indices. > > The temporary 'connector' pointer seems to be redundant as we can directly > fill the connector_set elements and thus avoid unnecessary NULL > assignments and checks. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: b164d31f50b2 ("drm/modes: add connector reference counting. (v2)") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> I'm sorry for bothering everyone with this issue, but status of the patch here [1] is still 'New', and I have no means to deduce whether the subsystem maintainers didn't have time to review (it is totally understandable as the amount of patches is enormous) or the patch was missed somehow. [1]: https://patchwork.kernel.org/project/dri-devel/patch/20230721101600.4392-1-pchelkin@ispras.ru/
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index df9bf3c9206e..2a29c3cf12de 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -709,7 +709,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, struct drm_mode_crtc *crtc_req = data; struct drm_crtc *crtc; struct drm_plane *plane; - struct drm_connector **connector_set = NULL, *connector; + struct drm_connector **connector_set = NULL; struct drm_framebuffer *fb = NULL; struct drm_display_mode *mode = NULL; struct drm_mode_set set; @@ -852,25 +852,22 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, } for (i = 0; i < crtc_req->count_connectors; i++) { - connector_set[i] = NULL; set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr; if (get_user(out_id, &set_connectors_ptr[i])) { ret = -EFAULT; goto out; } - connector = drm_connector_lookup(dev, file_priv, out_id); - if (!connector) { + connector_set[i] = drm_connector_lookup(dev, file_priv, out_id); + if (!connector_set[i]) { DRM_DEBUG_KMS("Connector id %d unknown\n", out_id); ret = -ENOENT; goto out; } DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", - connector->base.id, - connector->name); - - connector_set[i] = connector; + connector_set[i]->base.id, + connector_set[i]->name); } } @@ -891,12 +888,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, if (fb) drm_framebuffer_put(fb); - if (connector_set) { - for (i = 0; i < crtc_req->count_connectors; i++) { - if (connector_set[i]) - drm_connector_put(connector_set[i]); - } - } + if (connector_set) + while (--i >= 0) + drm_connector_put(connector_set[i]); kfree(connector_set); drm_mode_destroy(dev, mode);
Inside drm_mode_setcrtc() connector_set is allocated using kmalloc_array() so its values are uninitialized. When filling this array with actual pointers to drm connector objects, an error caused with invalid ioctl request data may occur leading us to put references to already taken objects. However, the last elements of the array are left uninitialized which makes drm_connector_put() to be called with an invalid argument. We can obviously just initialize the array with kcalloc() but the current fix chose a slightly different way. The index of failing array element is known so just put references to the array members with lower indices. The temporary 'connector' pointer seems to be redundant as we can directly fill the connector_set elements and thus avoid unnecessary NULL assignments and checks. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: b164d31f50b2 ("drm/modes: add connector reference counting. (v2)") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> --- drivers/gpu/drm/drm_crtc.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)