diff mbox series

video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit()

Message ID 05acdda8-dc1c-5119-4326-96eed24bea0c@i-love.sakura.ne.jp (mailing list archive)
State New
Headers show
Series video: fbdev: vga16fb: fix OOB write in vga16fb_imageblit() | expand

Commit Message

Tetsuo Handa May 14, 2021, 4:19 p.m. UTC
syzbot is reporting that a local user with the framebuffer console can
crash the kernel [1], for ioctl(VT_RESIZE) allows a TTY to set arbitrary
rows/columns values regardless of amount of memory reserved for
the graphical screen.

----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/kd.h>
#include <linux/vt.h>

int main(int argc, char *argv[])
{
        const int fd = open("/dev/char/4:1", O_RDWR);
        struct vt_sizes vt = { 0x4100, 2 };

        ioctl(fd, KDSETMODE, KD_GRAPHICS);
        ioctl(fd, VT_RESIZE, &vt);
        ioctl(fd, KDSETMODE, KD_TEXT);
        return 0;
}
----------

Currently it is impossible to control upper limit of rows/columns values
based on amount of memory reserved for the graphical screen, for
resize_screen() calls vc->vc_sw->con_resize() only if vc->vc_mode is not
already KD_GRAPHICS. I don't know the reason, and this condition predates
the git history. Even if it turns out to be safe to always call this
callback, we will need to involve another callback via "struct fb_ops" for
checking the upper limits from fbcon_resize(). As a result, we will need
to modify

 drivers/tty/vt/vt.c
 drivers/video/fbdev/core/fbcon.c
 drivers/video/fbdev/vga16fb.c
 include/linux/fb.h

files only for checking rows/columns values passed to ioctl(VT_RESIZE)
request.

Therefore, instead of introducing such a complicated callback chain, avoid
this problem by simply checking whether the address to read or write is in
[VGA_FB_PHYS, VGA_FB_PHYS + VGA_FB_PHYS_LEN) range.

[1] https://syzkaller.appspot.com/bug?extid=1f29e126cf461c4de3b3

Reported-by: syzbot <syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+1f29e126cf461c4de3b3@syzkaller.appspotmail.com>
---
 drivers/video/fbdev/vga16fb.c | 54 +++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 18 deletions(-)

Comments

Linus Torvalds May 14, 2021, 5:29 p.m. UTC | #1
On Fri, May 14, 2021 at 9:20 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Currently it is impossible to control upper limit of rows/columns values
> based on amount of memory reserved for the graphical screen, for
> resize_screen() calls vc->vc_sw->con_resize() only if vc->vc_mode is not
> already KD_GRAPHICS

Honestly, the saner approach would seem to be to simply error out if
vc_mode is KD_GRAPHICS.

Doing VT_RESIZE while in KD_GRAPHICS mode seems _very_ questionable,
and is clearly currently very buggy.

So why not just say "that clearly already doesn't work, so make it
explicitly not permitted"?

              Linus
Linus Torvalds May 14, 2021, 5:37 p.m. UTC | #2
On Fri, May 14, 2021 at 10:29 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So why not just say "that clearly already doesn't work, so make it
> explicitly not permitted"?

IOW, something like this would seem fairly simple and straightforward:

  diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
  index 01645e87b3d5..f24e627b7402 100644
  --- a/drivers/tty/vt/vt.c
  +++ b/drivers/tty/vt/vt.c
  @@ -1171,8 +1171,13 @@ static inline int resize_screen(struct
vc_data *vc, int width, int height,
          /* Resizes the resolution of the display adapater */
          int err = 0;

  -       if (vc->vc_mode != KD_GRAPHICS && vc->vc_sw->con_resize)
  +       if (vc->vc_sw->con_resize) {
  +               // If we have a resize function but are in KD_GRAPHICS mode,
  +               // we can't actually do a resize and need to error out.
  +               if (vc->vc_mode == KD_GRAPHICS)
  +                       return -EINVAL;
                  err = vc->vc_sw->con_resize(vc, width, height, user);
  +       }

          return err;
   }

not tested, but it looks ObviouslyCorrect(tm), and since we know the
old case didn't work right, it seems very safe to do.

           Linus
Linus Torvalds May 14, 2021, 6:23 p.m. UTC | #3
On Fri, May 14, 2021 at 10:37 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> IOW, something like this would seem fairly simple and straightforward:

Proper patch in case syzbot can test this..

                  Linus
Maciej W. Rozycki May 14, 2021, 8:25 p.m. UTC | #4
On Fri, 14 May 2021, Linus Torvalds wrote:

> > Currently it is impossible to control upper limit of rows/columns values
> > based on amount of memory reserved for the graphical screen, for
> > resize_screen() calls vc->vc_sw->con_resize() only if vc->vc_mode is not
> > already KD_GRAPHICS
> 
> Honestly, the saner approach would seem to be to simply error out if
> vc_mode is KD_GRAPHICS.
> 
> Doing VT_RESIZE while in KD_GRAPHICS mode seems _very_ questionable,
> and is clearly currently very buggy.

 I haven't looked into it any further beyond tracking down (again, using 
the LMO tree) the originating change as the other fix took precedence.  It 
came with:

commit 094e0a9cdbdf1e11a28dd756a6cbd750b6303d10
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Sun Jun 1 12:07:37 2003 +0000

    Merge with Linux 2.5.51

along with framebuffer console support:

+inline int resize_screen(int currcons, int width, int height)
+{
+	/* Resizes the resolution of the display adapater */
+	int err = 0;
+
+	if (vcmode != KD_GRAPHICS && sw->con_resize)
+		err = sw->con_resize(vc_cons[currcons].d, width, height);
+	return err;
+}
+

A handler for fbcon was added shortly afterwards with:

commit bab384bdbe279efd7acc2146ef13b0b0395b2a42
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Tue Jun 3 17:04:10 2003 +0000

    Merge with Linux 2.5.59.

however vgacon didn't have a handler for it until commit 28254d439b8c 
("[PATCH] vga text console and stty cols/rows") two years later only.

 Overall I think it does make sense to resize the text console at any 
time, even if the visible console (VT) chosen is in the graphics mode, as 
my understanding (and experience at least with vgacon) is that resizing 
the console applies globally across all the VTs.  So the intent of the 
original change appears valid to me, and the choice not to reprogram the 
visible console and only store the settings for a future use if it's in 
the graphics mode correct.

 Which means any bug triggered here needs to be fixed elsewhere rather 
than by making the request fail.

 NB for fbcon the usual ioctl to resize the console is FBIOPUT_VSCREENINFO 
rather than VT_RESIZEX; fbset(8) uses it, and I actually experimented with 
it and a TGA-like (SFB+) framebuffer when at my lab last time, as Linux is 
kind enough to know how to fiddle with its clockchip.  It works just fine.

  Maciej
Linus Torvalds May 14, 2021, 8:32 p.m. UTC | #5
On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
>  Overall I think it does make sense to resize the text console at any
> time, even if the visible console (VT) chosen is in the graphics mode,

It might make sense, but only if we call the function to update the
low-level data.

Not calling it, and then starting to randomly use the (wrong)
geometry, and just limiting it so that it's all within the buffer -
THAT does not make sense.

So I think your patch is fundamentally wrong. It basically says "let's
use random stale incorrect data, but just make sure that the end
result is still within the allocated buffer".

My patch is at least conceptually sane.

An alternative would be to just remove the "vcmode != KD_GRAPHICS"
check entirely, and always call con_resize() to update the low-level
data, but honestly, that seems very likelty to break something very
fundamentally, since it's not how any of fbcon has ever been tested,

Another alternative would be to just delay the resize to when vcmode
is put back to text mode again. That sounds somewhat reasonable to me,
but it's a pretty big thing.

But no, your patch to just "knowingly use entirely wrong values, then
add a limit check because we know the values are possibly garbage and
not consistent with reality" is simply not acceptable.

              Linus
Linus Torvalds May 14, 2021, 9:10 p.m. UTC | #6
On Fri, May 14, 2021 at 1:32 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.

Actually thinking more about that option, it sounds horrible. It would
mean that we'd continue to use the old geometry for the actual VC
buffers for a random time, and then change it to the new geometry at
some arbitrary point.

So I think the only reasonable approach (apart from just my "don't do
that then") might be to just always call ->con_resize().

There are only actually three cases of "->con_resize()", so it might
not be too bad.

Looking at it, both sisusbcon_resize() and vgacon_resize() seem to be
trivially fine in KD_GRAPHICS mode.

vgacon already seems to have that "!vga_is_gfx" test, and does
vgacon_doresize() at vgacon_switch(). It might need to add a
vgacon_doresize() to the vgacon_blank() case 0 code so that it
actually does the right thing when going back to KD_TEXT mode.

And fbcon_resize() looks like it might be mostly ok with it too.
Again, there is a con_is_visible() test, and I suspect that might need
to be changed to

        if (con_is_visible(vc) && vc->vc_mode == KD_TEXT)

instead,  but it doesn't look _too_ bad.

So I think just removing the "vc->vc_mode != KD_GRAPHICS" test from
resize_screen() might be the way to go. That way, the low-level data
structures actually are in sync with the resize, and the "out of
bounds" bug should never happen.

Would you mind testing that?

               Linus
Tetsuo Handa May 15, 2021, 12:45 a.m. UTC | #7
On 2021/05/15 5:25, Maciej W. Rozycki wrote:
>  NB for fbcon the usual ioctl to resize the console is FBIOPUT_VSCREENINFO 
> rather than VT_RESIZEX; fbset(8) uses it, and I actually experimented with 
> it and a TGA-like (SFB+) framebuffer when at my lab last time, as Linux is 
> kind enough to know how to fiddle with its clockchip.  It works just fine.

fbcon_update_vcs() from FBIOPUT_VSCREENINFO is no-op if vc->vc_mode != KD_TEXT
(which is equivalent to "if vc->vc_mode == KD_GRAPHICS" because KD_TEXT0/KD_TEXT1
are treated as KD_TEXT). Then, maybe it is OK to let resize_screen() return -EINVAL
in order to make vc_do_resize() request fail if vc->vc_mode == KD_GRAPHICS.

>  Overall I think it does make sense to resize the text console at any 
> time, even if the visible console (VT) chosen is in the graphics mode, as 
> my understanding (and experience at least with vgacon) is that resizing 
> the console applies globally across all the VTs.  So the intent of the 
> original change appears valid to me, and the choice not to reprogram the 
> visible console and only store the settings for a future use if it's in 
> the graphics mode correct.
>
>  Which means any bug triggered here needs to be fixed elsewhere rather 
> than by making the request fail.

Since syzbot does not trigger this problem with Linus's patch, I think we can
try Linus's patch with

  pr_info_once("Resizing text console while in graphical mode is ignored. Please report if you need this.\n");

added in order to see if somebody wants "only store the settings for a future use".
Maciej W. Rozycki May 15, 2021, 4:11 p.m. UTC | #8
On Fri, 14 May 2021, Linus Torvalds wrote:

> >  Overall I think it does make sense to resize the text console at any
> > time, even if the visible console (VT) chosen is in the graphics mode,
> 
> It might make sense, but only if we call the function to update the
> low-level data.
> 
> Not calling it, and then starting to randomly use the (wrong)
> geometry, and just limiting it so that it's all within the buffer -
> THAT does not make sense.
> 
> So I think your patch is fundamentally wrong. It basically says "let's
> use random stale incorrect data, but just make sure that the end
> result is still within the allocated buffer".

 I guess you mean Tetsuo-san's patch, right?  I haven't sent any in this 
discussion.

> My patch is at least conceptually sane.
> 
> An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> check entirely, and always call con_resize() to update the low-level
> data, but honestly, that seems very likelty to break something very
> fundamentally, since it's not how any of fbcon has ever been tested,

 Umm, there isn't much to change as far as console data structures are 
concerned with a resize: obviously the width and the height, which affect 
the size of the character/attribute buffer, and maybe some cursor data 
such as the size and screen coordinates.

 For vgacon we have:

	if (con_is_visible(c) && !vga_is_gfx) /* who knows */
		vgacon_doresize(c, width, height);

in `vgacon_resize' already, following all the sanity checks, so the CRTC 
isn't poked at if `vga_is_gfx', exactly as we want.

 I can see fbcon does not have equivalent code and instead has relied on 
the KD_GRAPHICS check made by the caller.  Which I think has been a bug 
since fbcon's inception.  Instead I think `fbcon_resize' ought to make all 
the sanity checks I can see it does and only then check for KD_GRAPHICS 
and if so, then exit without poking at hardware.  Then upon exit from the 
gfx mode the `fb_set_var' call made from `fbcon_blank' will DTRT.

 I can try verifying the latter hypothesis, though my framebuffer setups 
(with DECstation hardware) have always been somewhat incomplete.  I do 
believe I have a MIPS fbdev X server binary somewhere to fiddle with, 
which should work with that TGA/SFB+ video adapter I mentioned before.

> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.

 Methinks it works exactly like that already.  On exit from the graphics 
mode (a VT switch or gfx program termination) hardware is reprogrammed 
according to the console geometry previously set.  We just must not break 
it.

  Maciej
Daniel Vetter May 17, 2021, 1:07 p.m. UTC | #9
On Fri, May 14, 2021 at 10:33 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> >  Overall I think it does make sense to resize the text console at any
> > time, even if the visible console (VT) chosen is in the graphics mode,
>
> It might make sense, but only if we call the function to update the
> low-level data.
>
> Not calling it, and then starting to randomly use the (wrong)
> geometry, and just limiting it so that it's all within the buffer -
> THAT does not make sense.
>
> So I think your patch is fundamentally wrong. It basically says "let's
> use random stale incorrect data, but just make sure that the end
> result is still within the allocated buffer".
>
> My patch is at least conceptually sane.
>
> An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> check entirely, and always call con_resize() to update the low-level
> data, but honestly, that seems very likelty to break something very
> fundamentally, since it's not how any of fbcon has ever been tested,

Just an aside: I think with fbdev drivers this would go boom, because
you'd have fbcon interferring with a direct /dev/fb/* user.

But if your fbdev driver is actually a drm modeset driver, then we
have additional limitations: If the userspace accesses the display
through /dev/dri/card0, then the kernel blocks all access through
/dev/fb/* (including fbcon) to the actual display (it only goes into
the buffer used for fbdev emulation). And everything would be fine.

Also generally you'd get away with this even in problematic cases,
since usually you resize your console when looking at it, not when X
or something else is using your fbdev direct access.

The one thing that's left out here a bit in the cold is userspace
modeset drivers in X. Those would get hosed. But also, we stopped
supporting those in at least i915/amd/radeon/nouveau drivers,
automatically falling back to the fbdev stuff in most cases (with or
without the drm drivers underneath that), and no one screamed. So
probably not many users left.

So I /think/ we could wager this, if it's the least intrusive fix from
the kernel pov. But it has some risks that we need to revert again if
we break some of the really old use-cases here.

Cheers, Daniel

> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.
>
> But no, your patch to just "knowingly use entirely wrong values, then
> add a limit check because we know the values are possibly garbage and
> not consistent with reality" is simply not acceptable.
>
>               Linus
Daniel Vetter May 17, 2021, 1:10 p.m. UTC | #10
On Mon, May 17, 2021 at 3:07 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, May 14, 2021 at 10:33 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> > >
> > >  Overall I think it does make sense to resize the text console at any
> > > time, even if the visible console (VT) chosen is in the graphics mode,
> >
> > It might make sense, but only if we call the function to update the
> > low-level data.
> >
> > Not calling it, and then starting to randomly use the (wrong)
> > geometry, and just limiting it so that it's all within the buffer -
> > THAT does not make sense.
> >
> > So I think your patch is fundamentally wrong. It basically says "let's
> > use random stale incorrect data, but just make sure that the end
> > result is still within the allocated buffer".
> >
> > My patch is at least conceptually sane.
> >
> > An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> > check entirely, and always call con_resize() to update the low-level
> > data, but honestly, that seems very likelty to break something very
> > fundamentally, since it's not how any of fbcon has ever been tested,
>
> Just an aside: I think with fbdev drivers this would go boom, because
> you'd have fbcon interferring with a direct /dev/fb/* user.

Boom here means a bit of screen corruption, because fbcon overdraws
your X sessions. Fixed by the next redraw of X.

> But if your fbdev driver is actually a drm modeset driver, then we
> have additional limitations: If the userspace accesses the display
> through /dev/dri/card0, then the kernel blocks all access through
> /dev/fb/* (including fbcon) to the actual display (it only goes into
> the buffer used for fbdev emulation). And everything would be fine.
>
> Also generally you'd get away with this even in problematic cases,
> since usually you resize your console when looking at it, not when X
> or something else is using your fbdev direct access.
>
> The one thing that's left out here a bit in the cold is userspace
> modeset drivers in X. Those would get hosed. But also, we stopped
> supporting those in at least i915/amd/radeon/nouveau drivers,
> automatically falling back to the fbdev stuff in most cases (with or
> without the drm drivers underneath that), and no one screamed. So
> probably not many users left.

This one could lead to incosistent hw state, which would be worse.

> So I /think/ we could wager this, if it's the least intrusive fix from
> the kernel pov. But it has some risks that we need to revert again if
> we break some of the really old use-cases here.

Cheers, Daniel

> > Another alternative would be to just delay the resize to when vcmode
> > is put back to text mode again. That sounds somewhat reasonable to me,
> > but it's a pretty big thing.
> >
> > But no, your patch to just "knowingly use entirely wrong values, then
> > add a limit check because we know the values are possibly garbage and
> > not consistent with reality" is simply not acceptable.
> >
> >               Linus
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox series

Patch

diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index e2757ff1c23d..13732a3b1d69 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -98,6 +98,18 @@  static const struct fb_fix_screeninfo vga16fb_fix = {
 	.accel		= FB_ACCEL_NONE
 };
 
+/*
+ * Verify that the address to read or write is in [VGA_FB_PHYS, VGA_FB_PHYS + VGA_FB_PHYS_LEN)
+ * range, for ioctl(VT_RESIZE) allows a TTY to set arbitrary rows/columns values which will crash
+ * the kernel due to out of bounds access when trying to redraw the screen.
+ */
+static inline bool is_valid_iomem(const struct fb_info *info, const char __iomem *where)
+{
+	return info->screen_base <= where && where < info->screen_base + VGA_FB_PHYS_LEN;
+}
+
+#define IS_SAFE(where) is_valid_iomem(info, (where))
+
 /* The VGA's weird architecture often requires that we read a byte and
    write a byte to the same location.  It doesn't matter *what* byte
    we write, however.  This is because all the action goes on behind
@@ -851,7 +863,7 @@  static void vga_8planes_fillrect(struct fb_info *info, const struct fb_fillrect
                         int x;
 
                         /* we can do memset... */
-                        for (x = width; x > 0; --x) {
+			for (x = width; x > 0 && IS_SAFE(where); --x) {
                                 writeb(rect->color, where);
                                 where++;
                         }
@@ -864,7 +876,7 @@  static void vga_8planes_fillrect(struct fb_info *info, const struct fb_fillrect
                 oldop = setop(0x18);
                 oldsr = setsr(0xf);
                 setmask(0x0F);
-                for (y = 0; y < rect->height; y++) {
+		for (y = 0; y < rect->height && IS_SAFE(where) && IS_SAFE(where + 1); y++) {
                         rmw(where);
                         rmw(where+1);
                         where += info->fix.line_length;
@@ -919,7 +931,7 @@  static void vga16fb_fillrect(struct fb_info *info, const struct fb_fillrect *rec
 				setmask(0xff);
 
 				while (height--) {
-					for (x = 0; x < width; x++) {
+					for (x = 0; x < width && IS_SAFE(dst); x++) {
 						writeb(0, dst);
 						dst++;
 					}
@@ -935,7 +947,7 @@  static void vga16fb_fillrect(struct fb_info *info, const struct fb_fillrect *rec
 
 				setmask(0xff);
 				while (height--) {
-					for (x = 0; x < width; x++) {
+					for (x = 0; x < width && IS_SAFE(dst); x++) {
 						rmw(dst);
 						dst++;
 					}
@@ -975,7 +987,7 @@  static void vga_8planes_copyarea(struct fb_info *info, const struct fb_copyarea
                 dest = info->screen_base + dx + area->dy * info->fix.line_length;
                 src = info->screen_base + sx + area->sy * info->fix.line_length;
                 while (height--) {
-                        for (x = 0; x < width; x++) {
+			for (x = 0; x < width && IS_SAFE(src) && IS_SAFE(dest); x++) {
                                 readb(src);
                                 writeb(0, dest);
                                 src++;
@@ -991,7 +1003,7 @@  static void vga_8planes_copyarea(struct fb_info *info, const struct fb_copyarea
                 src = info->screen_base + sx + width +
 			(area->sy + height - 1) * info->fix.line_length;
                 while (height--) {
-                        for (x = 0; x < width; x++) {
+			for (x = 0; x < width && IS_SAFE(src - 1) && IS_SAFE(dest - 1); x++) {
                                 --src;
                                 --dest;
                                 readb(src);
@@ -1065,7 +1077,7 @@  static void vga16fb_copyarea(struct fb_info *info, const struct fb_copyarea *are
 				dst = info->screen_base + (dx/8) + dy * info->fix.line_length;
 				src = info->screen_base + (sx/8) + sy * info->fix.line_length;
 				while (height--) {
-					for (x = 0; x < width; x++) {
+					for (x = 0; x < width && IS_SAFE(src) && IS_SAFE(dst); x++) {
 						readb(src);
 						writeb(0, dst);
 						dst++;
@@ -1080,7 +1092,7 @@  static void vga16fb_copyarea(struct fb_info *info, const struct fb_copyarea *are
 				src = info->screen_base + (sx/8) + width + 
 					(sy + height  - 1) * info->fix.line_length;
 				while (height--) {
-					for (x = 0; x < width; x++) {
+					for (x = 0; x < width && IS_SAFE(src - 1) && IS_SAFE(dst - 1); x++) {
 						dst--;
 						src--;
 						readb(src);
@@ -1130,13 +1142,15 @@  static void vga_8planes_imageblit(struct fb_info *info, const struct fb_image *i
         where = info->screen_base + dx + image->dy * info->fix.line_length;
 
         setmask(0xff);
-        writeb(image->bg_color, where);
-        readb(where);
+	if (IS_SAFE(where)) {
+		writeb(image->bg_color, where);
+		readb(where);
+	}
         selectmask();
         setmask(image->fg_color ^ image->bg_color);
         setmode(0x42);
         setop(0x18);
-        for (y = 0; y < image->height; y++, where += info->fix.line_length)
+	for (y = 0; y < image->height && IS_SAFE(where); y++, where += info->fix.line_length)
                 writew(transl_h[cdat[y]&0xF] | transl_l[cdat[y] >> 4], where);
         setmask(oldmask);
         setsr(oldsr);
@@ -1165,14 +1179,16 @@  static void vga_imageblit_expand(struct fb_info *info, const struct fb_image *im
 				selectmask();
 				
 				setmask(0xff);
-				writeb(image->bg_color, where);
-				rmb();
-				readb(where); /* fill latches */
+				if (IS_SAFE(where)) {
+					writeb(image->bg_color, where);
+					rmb();
+					readb(where); /* fill latches */
+				}
 				setmode(3);
 				wmb();
 				for (y = 0; y < image->height; y++) {
 					dst = where;
-					for (x = image->width/8; x--;) 
+					for (x = image->width/8; x-- && IS_SAFE(dst);)
 						writeb(*cdat++, dst++);
 					where += info->fix.line_length;
 				}
@@ -1187,7 +1203,7 @@  static void vga_imageblit_expand(struct fb_info *info, const struct fb_image *im
 				setmask(0xff);
 				for (y = 0; y < image->height; y++) {
 					dst = where;
-					for (x=image->width/8; x--;){
+					for (x = image->width/8 && IS_SAFE(dst); x--;) {
 						rmw(dst);
 						setcolor(image->fg_color);
 						selectmask();
@@ -1237,8 +1253,10 @@  static void vga_imageblit_color(struct fb_info *info, const struct fb_image *ima
 					setcolor(*cdat);
 					selectmask();
 					setmask(1 << (7 - (x % 8)));
-					fb_readb(dst);
-					fb_writeb(0, dst);
+					if (IS_SAFE(dst)) {
+						fb_readb(dst);
+						fb_writeb(0, dst);
+					}
 
 					cdat++;
 				}