|
| From: | BALATON Zoltan |
| Subject: | Re: [PATCH v2 5/7] sm501: Replace hand written implementation with pixman where possible |
| Date: | Wed, 27 May 2020 13:05:09 +0200 (CEST) |
| User-agent: | Alpine 2.22 (BSF 395 2020-01-19) |
Hello, On Wed, 27 May 2020, Gerd Hoffmann wrote:
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 ...
Hopefully we can make the checks correct eventually. I think for sm501 it should already be OK, I'll need to check ati-vga again because I think there may be still a mistake in that. (It does not help that every device encode these values differently in registers.)
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.
I prefer performance here if security can be achieved without loss of performance with correct checks so rather fix the checks until they are correct than do additional things in a loop.
+ 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 notYes, 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 ...
Pixman does left to right, top to bottom so we don't need special handling for such blits, they will work even for overlapping areas. Doing non overlapping blits should also work with whatever direction (but AmigaOS seems to use rtl as default even for non overlapping, maybe hardware prefers that or was easier to code somehow). The only case where pixman does not work is reverse direction overlapping areas which is checked here, although becuase of different strides and offsets it's hard to check exactly so we only do a crude check to see if the memory areas are overlapping at all. This should catch all bad cases and maybe some good ones but checking for those is probably as expensive as doing the blit instead. As you said this may not work in some cases but until we come across such cases I'd go with this simpler solution because otherwise we likely need to implement our own optimised blit routine.
Unlike ati-vga, sm501 does not have independent direction bits so rtl seems to mean both right to left and bottom to top. Ati-vga has different bit for bottom to top so those with left to right could still use pixman_blt calling it in a reverse counting loop for every line but I did not go for that optimisation yet. For sm501 there's no such option. Possible furher optimisation could be handling 1 pixel and small regions directly where the overhead of calling pixman may be bigger than the gain from its optimised routines but I would need to measure that for which I have no time.
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).
You should. It's seriously undocumented and using it seems to need digging the code or maybe I've missed all the wonderful documentation?
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.
To my knowledge the sm501 is only used on an SH4 machine, the sam460ex and to run MorphOS on mac99 but ati-vga is already better for the latter so these are not security critical in my opinion.
Regards, BALATON Zoltan
| [Prev in Thread] | Current Thread | [Next in Thread] |