2018-04-18 05:44:24

by Gerd Hoffmann

Subject: [PATCH 1/2] qxl: fix qxl_release_{map,unmap}

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 <[email protected]>
---
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

2018-04-20 07:49:59

by Daniel Vetter

Subject: Re: [PATCH 1/2] qxl: fix qxl_release_{map,unmap}

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 <[email protected]>

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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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