linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] qxl: fix qxl_release_{map,unmap}
       [not found] <20180418054257.15388-1-kraxel@redhat.com>
@ 2018-04-18  5:42 ` Gerd Hoffmann
  2018-04-20  7:48   ` Daniel Vetter
  2018-04-18  5:42 ` [PATCH 2/2] qxl: keep separate release_bo pointer Gerd Hoffmann
  1 sibling, 1 reply; 3+ messages in thread
From: Gerd Hoffmann @ 2018-04-18  5:42 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

s/PAGE_SIZE/PAGE_MASK/

Luckily release_offset is never larger than PAGE_SIZE, so the bug has no
bad side effects and managed to stay unnoticed for years that way ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c   | 4 ++--
 drivers/gpu/drm/qxl/qxl_release.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index e238a1a2ec..6cc9f3367f 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -182,9 +182,9 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		goto out_free_reloc;
 
 	/* TODO copy slow path code from i915 */
-	fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_SIZE));
+	fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK));
 	unwritten = __copy_from_user_inatomic_nocache
-		(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_SIZE),
+		(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_MASK),
 		 u64_to_user_ptr(cmd->command), cmd->command_size);
 
 	{
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 5d84a66fed..a0b4244d28 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -411,10 +411,10 @@ union qxl_release_info *qxl_release_map(struct qxl_device *qdev,
 	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
 	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
 
-	ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_SIZE);
+	ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_MASK);
 	if (!ptr)
 		return NULL;
-	info = ptr + (release->release_offset & ~PAGE_SIZE);
+	info = ptr + (release->release_offset & ~PAGE_MASK);
 	return info;
 }
 
@@ -426,7 +426,7 @@ void qxl_release_unmap(struct qxl_device *qdev,
 	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
 	void *ptr;
 
-	ptr = ((void *)info) - (release->release_offset & ~PAGE_SIZE);
+	ptr = ((void *)info) - (release->release_offset & ~PAGE_MASK);
 	qxl_bo_kunmap_atomic_page(qdev, bo, ptr);
 }
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [PATCH 2/2] qxl: keep separate release_bo pointer
       [not found] <20180418054257.15388-1-kraxel@redhat.com>
  2018-04-18  5:42 ` [PATCH 1/2] qxl: fix qxl_release_{map,unmap} Gerd Hoffmann
@ 2018-04-18  5:42 ` Gerd Hoffmann
  1 sibling, 0 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2018-04-18  5:42 UTC (permalink / raw)
  To: dri-devel
  Cc: Gerd Hoffmann, Dave Airlie, David Airlie,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU, open list

qxl expects that list_first_entry(release->bos) returns the first
element qxl added to the list.  ttm_eu_reserve_buffers() may reorder
the list though.

Add a release_bo field to struct qxl_release and use that instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_drv.h     |  1 +
 drivers/gpu/drm/qxl/qxl_cmd.c     |  6 ++----
 drivers/gpu/drm/qxl/qxl_release.c | 12 ++++++------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 00a1a66b05..864b456080 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -167,6 +167,7 @@ struct qxl_release {
 
 	int id;
 	int type;
+	struct qxl_bo *release_bo;
 	uint32_t release_offset;
 	uint32_t surface_release_id;
 	struct ww_acquire_ctx ticket;
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index c0fb52c6d4..01665b98c5 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -179,10 +179,9 @@ qxl_push_command_ring_release(struct qxl_device *qdev, struct qxl_release *relea
 			      uint32_t type, bool interruptible)
 {
 	struct qxl_command cmd;
-	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
 
 	cmd.type = type;
-	cmd.data = qxl_bo_physical_address(qdev, to_qxl_bo(entry->tv.bo), release->release_offset);
+	cmd.data = qxl_bo_physical_address(qdev, release->release_bo, release->release_offset);
 
 	return qxl_ring_push(qdev->command_ring, &cmd, interruptible);
 }
@@ -192,10 +191,9 @@ qxl_push_cursor_ring_release(struct qxl_device *qdev, struct qxl_release *releas
 			     uint32_t type, bool interruptible)
 {
 	struct qxl_command cmd;
-	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
 
 	cmd.type = type;
-	cmd.data = qxl_bo_physical_address(qdev, to_qxl_bo(entry->tv.bo), release->release_offset);
+	cmd.data = qxl_bo_physical_address(qdev, release->release_bo, release->release_offset);
 
 	return qxl_ring_push(qdev->cursor_ring, &cmd, interruptible);
 }
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index a0b4244d28..7cb2145772 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -173,6 +173,7 @@ qxl_release_free_list(struct qxl_release *release)
 		list_del(&entry->tv.head);
 		kfree(entry);
 	}
+	release->release_bo = NULL;
 }
 
 void
@@ -296,7 +297,6 @@ int qxl_alloc_surface_release_reserved(struct qxl_device *qdev,
 {
 	if (surface_cmd_type == QXL_SURFACE_CMD_DESTROY && create_rel) {
 		int idr_ret;
-		struct qxl_bo_list *entry = list_first_entry(&create_rel->bos, struct qxl_bo_list, tv.head);
 		struct qxl_bo *bo;
 		union qxl_release_info *info;
 
@@ -304,8 +304,9 @@ 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;
-		bo = to_qxl_bo(entry->tv.bo);
+		bo = create_rel->release_bo;
 
+		(*release)->release_bo = bo;
 		(*release)->release_offset = create_rel->release_offset + 64;
 
 		qxl_release_list_add(*release, bo);
@@ -365,6 +366,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 
 	bo = qxl_bo_ref(qdev->current_release_bo[cur_idx]);
 
+	(*release)->release_bo = bo;
 	(*release)->release_offset = qdev->current_release_bo_offset[cur_idx] * release_size_per_bo[cur_idx];
 	qdev->current_release_bo_offset[cur_idx]++;
 
@@ -408,8 +410,7 @@ union qxl_release_info *qxl_release_map(struct qxl_device *qdev,
 {
 	void *ptr;
 	union qxl_release_info *info;
-	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
-	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
+	struct qxl_bo *bo = release->release_bo;
 
 	ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_MASK);
 	if (!ptr)
@@ -422,8 +423,7 @@ void qxl_release_unmap(struct qxl_device *qdev,
 		       struct qxl_release *release,
 		       union qxl_release_info *info)
 {
-	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
-	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
+	struct qxl_bo *bo = release->release_bo;
 	void *ptr;
 
 	ptr = ((void *)info) - (release->release_offset & ~PAGE_MASK);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] qxl: fix qxl_release_{map,unmap}
  2018-04-18  5:42 ` [PATCH 1/2] qxl: fix qxl_release_{map,unmap} Gerd Hoffmann
@ 2018-04-20  7:48   ` Daniel Vetter
  0 siblings, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2018-04-20  7:48 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: dri-devel, David Airlie, Dave Airlie, open list,
	open list:DRM DRIVER FOR QXL VIRTUAL GPU

On Wed, Apr 18, 2018 at 07:42:56AM +0200, Gerd Hoffmann wrote:
> s/PAGE_SIZE/PAGE_MASK/
> 
> Luckily release_offset is never larger than PAGE_SIZE, so the bug has no
> bad side effects and managed to stay unnoticed for years that way ...
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Sweeet. Since the buggy code uses the same expression for page frame and
offset I don't think there's a security bug. You might still want to cc:
stable (since without you defacto can't ever use this feature).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/qxl/qxl_ioctl.c   | 4 ++--
>  drivers/gpu/drm/qxl/qxl_release.c | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index e238a1a2ec..6cc9f3367f 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -182,9 +182,9 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>  		goto out_free_reloc;
>  
>  	/* TODO copy slow path code from i915 */
> -	fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_SIZE));
> +	fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK));
>  	unwritten = __copy_from_user_inatomic_nocache
> -		(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_SIZE),
> +		(fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_MASK),
>  		 u64_to_user_ptr(cmd->command), cmd->command_size);
>  
>  	{
> diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
> index 5d84a66fed..a0b4244d28 100644
> --- a/drivers/gpu/drm/qxl/qxl_release.c
> +++ b/drivers/gpu/drm/qxl/qxl_release.c
> @@ -411,10 +411,10 @@ union qxl_release_info *qxl_release_map(struct qxl_device *qdev,
>  	struct qxl_bo_list *entry = list_first_entry(&release->bos, struct qxl_bo_list, tv.head);
>  	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
>  
> -	ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_SIZE);
> +	ptr = qxl_bo_kmap_atomic_page(qdev, bo, release->release_offset & PAGE_MASK);
>  	if (!ptr)
>  		return NULL;
> -	info = ptr + (release->release_offset & ~PAGE_SIZE);
> +	info = ptr + (release->release_offset & ~PAGE_MASK);
>  	return info;
>  }
>  
> @@ -426,7 +426,7 @@ void qxl_release_unmap(struct qxl_device *qdev,
>  	struct qxl_bo *bo = to_qxl_bo(entry->tv.bo);
>  	void *ptr;
>  
> -	ptr = ((void *)info) - (release->release_offset & ~PAGE_SIZE);
> +	ptr = ((void *)info) - (release->release_offset & ~PAGE_MASK);
>  	qxl_bo_kunmap_atomic_page(qdev, bo, ptr);
>  }
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-04-20  7:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20180418054257.15388-1-kraxel@redhat.com>
2018-04-18  5:42 ` [PATCH 1/2] qxl: fix qxl_release_{map,unmap} Gerd Hoffmann
2018-04-20  7:48   ` Daniel Vetter
2018-04-18  5:42 ` [PATCH 2/2] qxl: keep separate release_bo pointer Gerd Hoffmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).