On Fri, 21 Jul 2023, Ziqi Zhao <astrajoan@xxxxxxxxx> wrote:
> The connector_set contains uninitialized values when allocated with
> kmalloc_array. However, in the "out" branch, the logic assumes that any
> element in connector_set would be equal to NULL if failed to
> initialize, which causes the bug reported by Syzbot. The fix is to use
> an extra variable to keep track of how many connectors are initialized
> indeed, and use that variable to decrease any refcounts in the "out"
> branch.
>From one uninit-value bug to another?
>
> Reported-by: syzbot+4fad2e57beb6397ab2fc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Ziqi Zhao <astrajoan@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_crtc.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index df9bf3c9206e..d718c17ab1e9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -715,8 +715,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> struct drm_mode_set set;
> uint32_t __user *set_connectors_ptr;
> struct drm_modeset_acquire_ctx ctx;
> - int ret;
> - int i;
> + int ret, i, num_connectors;
num_connectors is uninitialized.
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EOPNOTSUPP;
> @@ -851,6 +850,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> goto out;
> }
>
> + num_connectors = 0;
num_connectors is initialized only if crtc_req->count_connectors > 0.
> 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;
> @@ -871,6 +871,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> connector->name);
>
> connector_set[i] = connector;
> + num_connectors++;
> }
> }
>
> @@ -879,7 +880,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> set.y = crtc_req->y;
> set.mode = mode;
> set.connectors = connector_set;
> - set.num_connectors = crtc_req->count_connectors;
> + set.num_connectors = num_connectors;
num_connectors is used uninitialized if crtc_req->count_connectors <= 0.
BR,
Jani.
> set.fb = fb;
>
> if (drm_drv_uses_atomic_modeset(dev))
> @@ -892,7 +893,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> drm_framebuffer_put(fb);
>
> if (connector_set) {
> - for (i = 0; i < crtc_req->count_connectors; i++) {
> + for (i = 0; i < num_connectors; i++) {
> if (connector_set[i])
> drm_connector_put(connector_set[i]);
> }
--
Jani Nikula, Intel
