Re: [PATCH v2 5/7] sm501: Replace hand written implementation with pixman where possible

[Gerd Hoffmann <kraxel@redhat.com>]

On Thu, May 21, 2020 at 09:39:44PM +0200, BALATON Zoltan wrote:
> Besides being faster this should also prevent malicious guests to
> abuse 2D engine to overwrite data or cause a crash.

>          uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> -        uint8_t *src = s->local_mem + src_base;

> -                    val = *(_pixel_type *)&src[index_s];                      \

Well, the advantage of *not* using pixman is that you can easily switch
the code to use offsets instead of pointers, then apply the mask to the
*final* offset to avoid oob data access:

    val = *(_pixel_type*)(&s->local_mem[(s->twoD_source_base + index_s) & 0x03FFFFFF]);

> +        if ((rop_mode && rop == 0x5) || (!rop_mode && rop == 0x55)) {
> +            /* Invert dest, is there a way to do this with pixman? */

PIXMAN_OP_XOR maybe?

> +            if (rtl && ((db >= sb && db <= se) || (de >= sb && de <= se))) {
> +                /* regions may overlap: copy via temporary */

The usual way for a hardware blitter is to have a direction bit, i.e.
the guest os can ask to blit in top->bottom or bottom->top scanline
ordering.  The guest can use that to make sure the blit does not
overwrite things.  But note the guest can also intentionally use
overlapping regions, i.e. memset(0) the first scanline, then use a blit
with overlap to clear the whole screen.  The later will surely break if
you blit via temporary image ...

> +                pixman_blt((uint32_t *)&s->local_mem[src_base],
> +                           (uint32_t *)&s->local_mem[dst_base],
> +                           src_pitch * (1 << format) / sizeof(uint32_t),
> +                           dst_pitch * (1 << format) / sizeof(uint32_t),
> +                           8 * (1 << format), 8 * (1 << format),
> +                           src_x, src_y, dst_x, dst_y, width, height);

See above, i'm not convinced pixman is the best way here.
When using pixman I'd suggest:

  (1) src = pixman_image_create_bits_no_clear(...);
  (2) dst = pixman_image_create_bits_no_clear(...);
  (3) pixman_image_composite(PIXMAN_OP_SRC, src, NULL, dst, ...);
  (4) pixman_image_unref(src);
  (5) pixman_image_unref(dst);

pixman_blt() is probably doing basically the same.  The advantage of not
using pixman_blt() is that

  (a) you can also use pixman ops other than PIXMAN_OP_SRC, and
  (b) you can have a helper function for (1)+(2) which very carefully
      applies sanity checks to make sure the pixman image created stays
      completely inside s->local_mem.
  (c) you have the option to completely rearrange the code flow, for
      example update the src pixman image whenever the guest touches
      src_base or src_pitch or format instead of having a
      create/op/unref cycle on every blitter op.

> +        pixman_fill((uint32_t *)&s->local_mem[dst_base],
> +                    dst_pitch * (1 << format) / sizeof(uint32_t),
> +                    8 * (1 << format), dst_x, dst_y, width, height, color);

  (1) src = pixman_image_create_solid(...), otherwise same as above ;)

take care,
  Gerd