On Wed, 15 Apr 2020, Peter Maydell wrote:
On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
Zhang Zi Ming reported a heap overflow in the Drawing Engine of
the SM501 companion chip model, in particular in the COPY_AREA()
macro in sm501_2d_operation().
Add a simple check to avoid the heap overflow.
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index de0ab9d977..902acb3875 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
+ if (rtl && (src_x < operation_width || src_y < operation_height)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address (%i, %i)\n",
+ src_x, src_y);
+ return;
+ }
This does fix an issue, but I have a feeling that there are
other possible guest register value combinations that might
cause us to index off one end or the other of the local_mem.
That's what I've meant by it should be reimplemented eventually to fix all
possible problems but could not do that before 5.0. Since this is existing
bug ever since this device is first committed not patching it now is
probably not a big deal if this is not considered a security problem. (And
if it is then all the abort() calls are probably a problem too although
less serious.)
The SM501 datasheet is entirely unhelpful on this question, but
my suggestion is that we should convert the code so that instead
of operating directly on pointers into the middle of the local_mem
buffer all the accesses to local_mem go via functions which mask
off the high bits of the index. That effectively means that the
behaviour if we index off the end of the graphics memory is
that we just wrap round to the start of it. It should be fairly
easy to be confident that the code isn't accessing off the end
of the array and it might even be what the hardware actually does
(since it would correspond to 'use low bits of the address to
index the ram, ignore high bits')...
Does that make it even slower than it is already? I think it should rather
be changed to do what I've done in ati_2d.c and call optimised functions
to do the blit operation instead of implementing it directly. Then we'll
need checking parameters to avoid overflows. I may try to do that
eventually but don't know when will I have time for that so if there's
anyone who submits a patch fixing it some way before that that's OK too.
I also know about these missing ops that could be fixed:
sm501: rop3 mode with rop 99 is not supported.
sm501: rop3 mode with rop ee is not supported.
Regards,
BALATON Zoltan
