Re: [PATCH v3] drm/qxl: validate relocation destination offsets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/qxl: double PAGE_SIZE scaling in qxl_bo_kmap_atomic_page() fallback
- [High] drm/qxl: natural alignment check in qxl_process_single_command() breaks UAPI
- [High] drm/qxl: integer wrap and missing command bounds check for dst_handle == 0
--

commit ac35098698450b791455e5afa6c8c8c17114609f
Author: Zhou Qiuyue <n0ps1edzz@gmail.com>

drm/qxl: validate relocation destination offsets

This commit widens relocation destination offset types and validates
the destination offset and alignment against the destination buffer
object size in qxl_process_single_command().

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 591b026ceff9c..9a031bb3f9b0c 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c

[ ... ]

> @@ -231,6 +232,26 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>  			reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset;
>  		}
>  
> +		/*
> +		 * The relocation is applied with a plain write into the memory
> +		 * window of the destination object, so the user supplied offset
> +		 * must be validated against the object size.  Otherwise userspace
> +		 * can write anywhere in the mapping window set up by
> +		 * qxl_bo_kmap_atomic_page(), i.e. outside of the object.  The
> +		 * offset must also be aligned to the access size so the write
> +		 * cannot straddle a page boundary inside the window.
> +		 */
> +		reloc_size = (reloc.reloc_type == QXL_RELOC_TYPE_BO) ?
> +			     sizeof(uint64_t) : sizeof(uint32_t);
> +		if (!IS_ALIGNED(reloc_info[i].dst_offset, reloc_size) ||
> +		    reloc_info[i].dst_offset > reloc_info[i].dst_bo->tbo.base.size ||
> +		    reloc_size > reloc_info[i].dst_bo->tbo.base.size -
> +				 reloc_info[i].dst_offset) {
> +			DRM_DEBUG("reloc dst offset out of bounds\n");
> +			ret = -EINVAL;
> +			goto out_free_bos;
> +		}

[Severity: High]
Won't requiring dst_offset to be aligned to reloc_size (8 or 4 bytes) in
qxl_process_single_command() reject valid QXL draw commands from userspace
with -EINVAL?

In drivers/gpu/drm/qxl/qxl_dev.h, the QXL protocol structures are defined
under #pragma pack(push, 1):

drivers/gpu/drm/qxl/qxl_dev.h:
    struct qxl_drawable {
            union qxl_release_info release_info;
            uint32_t surface_id;
            uint8_t effect;
            uint8_t type;
            uint8_t self_bitmap;
            struct qxl_rect self_bitmap_area;
            struct qxl_rect bbox;
            struct qxl_clip clip;
            uint32_t mm_time;
            int32_t surfaces_dest[3];
            struct qxl_rect surfaces_rects[3];
            union {
                    struct qxl_fill fill;
                    struct qxl_opaque opaque;
                    struct qxl_copy copy;
                    ...

Because of the three uint8_t fields (effect, type, self_bitmap), subsequent
relocated fields inside struct qxl_drawable sit at unaligned byte offsets,
such as clip.data (offset 51), surfaces_dest[0..2] (offsets 63, 67, 71), and
union u fields like u.copy.src_bitmap (offset 123).

Similarly, when relocating into another buffer object (reloc.dst_handle != 0),
struct qxl_data_chunk places prev_chunk and next_chunk (8-byte QXLPHYSICAL
fields) at offsets 4 and 12:

drivers/gpu/drm/qxl/qxl_dev.h:
    struct qxl_data_chunk {
            uint32_t data_size;
            QXLPHYSICAL prev_chunk;
            QXLPHYSICAL next_chunk;
            uint8_t data[];
    };

and struct qxl_image places u.surface_image.surface_id at offset 18.

To prevent a relocation write from straddling a page boundary inside the
single page mapped by qxl_bo_kmap_atomic_page(), should this check that the
offset within the page does not cross PAGE_SIZE (for example,
offset_in_page(reloc_info[i].dst_offset) > PAGE_SIZE - reloc_size) rather
than requiring natural alignment via IS_ALIGNED()?

[Severity: High]
Also in qxl_process_single_command(), when reloc.dst_handle == 0, can
reloc.dst_offset + release->release_offset wrap around or write outside the
command's slot in cmd_bo?

In qxl_alloc_release_reserved(), cmd_bo is a PAGE_SIZE buffer object shared
across multiple 256-byte release slots at release->release_offset, with
union qxl_release_info stored at the start of each slot:

drivers/gpu/drm/qxl/qxl_release.c:qxl_alloc_release_reserved() {
    ...
    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]++;
    ...
}

When release->release_offset > 0, adding a large user-supplied
reloc.dst_offset can wrap around in uint64_t to a small offset before the
bounds check. Furthermore, checking only against cmd_bo->tbo.base.size
(PAGE_SIZE) instead of the command's range
[sizeof(union qxl_release_info), sizeof(union qxl_release_info) + cmd->command_size)
allows a relocation to overwrite the union qxl_release_info header or
adjacent release slots sharing cmd_bo.

>  
>  		/* reserve and validate the reloc dst bo */
>  		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
>  			ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, release,
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 313f6c30cac85..0df48141c2db7 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -204,7 +204,7 @@ int qxl_bo_pin_and_vmap(struct qxl_bo *bo, struct iosys_map *map)
>  }
>  
>  void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
> -			      struct qxl_bo *bo, int page_offset)
> +			      struct qxl_bo *bo, unsigned long page_offset)
>  {
>  	unsigned long offset;
>  	void *rptr;

[Severity: Critical]
When dst_bo is backed by TTM_PL_SYSTEM (such as a multi-page dumb BO created
via DRM_IOCTL_MODE_CREATE_DUMB with QXL_GEM_DOMAIN_CPU), can a relocation
with dst_offset >= PAGE_SIZE still write out of bounds in the fallback path
of qxl_bo_kmap_atomic_page()?

In apply_reloc() and apply_surf_reloc(), the caller passes a byte offset
masked with PAGE_MASK and does not check the return value for NULL:

drivers/gpu/drm/qxl/qxl_ioctl.c:apply_reloc() {
    ...
    reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK);
    *(uint64_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = qxl_bo_physical_address(qdev,
                                                                                  info->src_bo,
                                                                                  info->src_offset);
    ...
}

In qxl_bo_kmap_atomic_page(), the io_mapping_map_atomic_wc() path treats
page_offset as a byte offset, but the fallback path multiplies page_offset by
PAGE_SIZE a second time:

drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() {
    ...
    offset = bo->tbo.resource->start << PAGE_SHIFT;
    return io_mapping_map_atomic_wc(map, offset + page_offset);
fallback:
    if (bo->kptr) {
            rptr = bo->kptr + (page_offset * PAGE_SIZE);
            return rptr;
    }

    ret = qxl_bo_vmap_locked(bo, &bo_map);
    if (ret)
            return NULL;
    rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */

    rptr += page_offset * PAGE_SIZE;
    return rptr;
}

If dst_offset >= PAGE_SIZE (while still within dst_bo->tbo.base.size, passing
the new bounds check), doesn't page_offset * PAGE_SIZE advance rptr far past
the end of the buffer object (or return NULL if qxl_bo_vmap_locked() fails)?

[ ... ]

> -- 

—
🤖 Generated by Sashiko — Linux Kernel Patch Review
https://sashiko.dev
—

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179240154188.24976.18308245375219473922@gmail.com?part=1