[PATCH] drm/qxl: Handle failure to reference the surface release buffer

[PATCH] drm/qxl: Handle failure to reference the surface release buffer

[Alex Kazinsky]

When allocating a surface destroy release, qxl_release_list_add()
takes a reference to the buffer shared with the create release.
If allocation of the list entry fails, the error is ignored and
the destroy command is submitted without holding that reference.

Once the current release buffer has changed and the create release
has been freed, the buffer may be freed while the destroy release
still points to it. Garbage collection can then access the freed
buffer through qxl_release_map().

Check the return value, free the new release and propagate the error
before submitting the command. Increment release_count after successful
release allocation to balance the decrement in qxl_release_free() on
both the error and normal completion paths.

Found by Linux Verification Center (linuxtesting.org) with SVACE static
analysis tool.

Fixes: 8002db6336dd ("qxl: convert qxl driver to proper use for reservations")
Cc: stable@vger.kernel.org
Signed-off-by: Alex Kazinsky <a.kazinsky@ideco.ru>
---
 drivers/gpu/drm/qxl/qxl_release.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 06979d0e8a9f..342c3600d425 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -262,6 +262,7 @@ int qxl_alloc_surface_release_reserved(struct qxl_device *qdev,
 {
 	if (surface_cmd_type == QXL_SURFACE_CMD_DESTROY && create_rel) {
 		int idr_ret;
+		int ret;
 		struct qxl_bo *bo;
 		union qxl_release_info *info;
 
@@ -269,12 +270,17 @@ int qxl_alloc_surface_release_reserved(struct qxl_device *qdev,
 		idr_ret = qxl_release_alloc(qdev, QXL_RELEASE_SURFACE_CMD, release);
 		if (idr_ret < 0)
 			return idr_ret;
+		atomic_inc(&qdev->release_count);
 		bo = create_rel->release_bo;
 
 		(*release)->release_bo = bo;
 		(*release)->release_offset = create_rel->release_offset + 64;
 
-		qxl_release_list_add(*release, bo);
+		ret = qxl_release_list_add(*release, bo);
+		if (ret) {
+			qxl_release_free(qdev, *release);
+			return ret;
+		}
 
 		info = qxl_release_map(qdev, *release);
 		info->id = idr_ret;
-- 
2.56.0

Re: [PATCH] drm/qxl: Handle failure to reference the surface release buffer

[sashiko-bot]

> When allocating a surface destroy release, qxl_release_list_add()
> takes a reference to the buffer shared with the create release.
> If allocation of the list entry fails, the error is ignored and
> the destroy command is submitted without holding that reference.
> 
> Once the current release buffer has changed and the create release
> has been freed, the buffer may be freed while the destroy release
> still points to it. Garbage collection can then access the freed
> buffer through qxl_release_map().
> 
> Check the return value, free the new release and propagate the error
> before submitting the command. Increment release_count after successful
> release allocation to balance the decrement in qxl_release_free() on
> both the error and normal completion paths.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE static
> analysis tool.
> 
> Fixes: 8002db6336dd ("qxl: convert qxl driver to proper use for reservations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alex Kazinsky <a.kazinsky@ideco.ru>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110658.4188477-1-a.kazinsky@ideco.ru?part=1