All of lore.kernel.org
 help / color / mirror / Atom feed
From: Fedor Pchelkin <pchelkin@ispras.ru>
To: David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Dave Airlie <airlied@redhat.com>
Cc: Fedor Pchelkin <pchelkin@ispras.ru>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Stone <daniels@collabora.com>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Alexey Khoroshilov <khoroshilov@ispras.ru>,
	lvc-project@linuxtesting.org
Subject: [PATCH] drm/crtc: do not release uninitialized connector reference
Date: Fri, 21 Jul 2023 13:15:59 +0300	[thread overview]
Message-ID: <20230721101600.4392-1-pchelkin@ispras.ru> (raw)

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(-)

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);
 
-- 
2.41.0


WARNING: multiple messages have this Message-ID (diff)
From: Fedor Pchelkin <pchelkin@ispras.ru>
To: David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Dave Airlie <airlied@redhat.com>
Cc: Daniel Stone <daniels@collabora.com>,
	Fedor Pchelkin <pchelkin@ispras.ru>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Alexey Khoroshilov <khoroshilov@ispras.ru>,
	lvc-project@linuxtesting.org
Subject: [PATCH] drm/crtc: do not release uninitialized connector reference
Date: Fri, 21 Jul 2023 13:15:59 +0300	[thread overview]
Message-ID: <20230721101600.4392-1-pchelkin@ispras.ru> (raw)

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(-)

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);
 
-- 
2.41.0


             reply	other threads:[~2023-07-21 10:24 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-21 10:15 Fedor Pchelkin [this message]
2023-07-21 10:15 ` [PATCH] drm/crtc: do not release uninitialized connector reference Fedor Pchelkin
2023-10-06 14:58 ` Fedor Pchelkin
2023-10-06 14:58   ` Fedor Pchelkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230721101600.4392-1-pchelkin@ispras.ru \
    --to=pchelkin@ispras.ru \
    --cc=airlied@gmail.com \
    --cc=airlied@redhat.com \
    --cc=daniel@ffwll.ch \
    --cc=daniels@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=khoroshilov@ispras.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lvc-project@linuxtesting.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.