qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH-for-5.0 v2] hw/display/sm501: Avoid heap overflow in sm501_2d


From: BALATON Zoltan
Subject: Re: [PATCH-for-5.0 v2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
Date: Wed, 15 Apr 2020 19:39:14 +0200 (CEST)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

On Wed, 15 Apr 2020, BALATON Zoltan wrote:
On Wed, 15 Apr 2020, Peter Maydell wrote:
On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <address@hidden> 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

As blits are common operation in several video cards, such as sm501, cirrus and ati-vga at least maybe we could also split off some common helpers to have one implementation of these which could be secured and optimised once and not have to fix it in every device separately. I don't volunteer to do that by maybe there's someone who wants to try that?

Regards,
BALATON Zoltan

reply via email to

[Prev in Thread] Current Thread [Next in Thread]