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

[Gerd Hoffmann]

  Hi,

> > 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:
> 
> The mask applied to src_base is not to prevent overflow but to implement
> register limits.

Yea, that was just a quick sketch to outline the idea without checking
all details.

> This patch basically does
> that by changing parameters to unsigned to prevent them being negative,
> checking values we multiply by to prevent them to be zero and then
> calculating first and last offset and check if they are within vram.

Well.  With cirrus this proved to be fragile.  The checks missed corner
cases and we've got a series of CVEs in the blitter code.  Switching to
offsets + masking every vram access (see commit ffaf85777828) stopped
that.

> (Unless
> of course I've made a mistake somewhere.)

Exactly ...

> This should prevent overflow with
> one check and does not need to apply a mask at every step. The vram size can
> also be different so it's not a fixed mask anyway.
> 
> If not using pixman then I'd need to reimplement optimised 2D ops that will
> likely never be as good as pixman and no point in doing it several times for
> every device model so I'd rather try to use pixman where possible unless a
> better library is available.

Yes, performance-wise pixman is clearly the better choice.  At the end
of the day it is a security vs performance trade off.

> > > +            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
> 
> Yes, this is the rtl above (right to left) and AmigaOS sets this most of the
> time so only relying on that to detect overlaps is not efficient.

Hmm, checking rtl like that doesn't look correct to me then.  When using
the blitter to move a window you have to set/clear rtl depending on
whenever you move the window up or down on the screen, and src+dst
regions can overlap in both cases ...

> > 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 ...
> 
> Fortunately no guest code seems to do that so unless we find one needing it
> I don't worry much about such rare cases.

Ok.

> > > +                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.
> 
> Actually not the same, pixman_blt is faster operating directly on pointers
> while we need all the pixman_image overhead to use pixman_image_composite.

Ok (I didn't check the pixman code).

Given the use case (run a computer museum ;) I think we can live with
the flaws of the pixman approach.  Security shouldn't be that much of an
issue here.  The behavior and blitter use pattern of the guests is known
too and unlikely to change.

take care,
  Gerd